Add featureRetire2017Amendments: #3368
Conversation
src/ripple/app/misc/README.md
Outdated
| a period of two weeks before being accepted. The following example outlines the | ||
| process of an Amendment from its conception to approval and usage. | ||
|
|
||
| * A community member makes proposes to change transaction processing in some |
There was a problem hiding this comment.
Typo? makes proposes -> proposes
| // For every retiring amendment, if we find it remove it from | ||
| // amendmentObject. | ||
| // | ||
| // Note that we can't remove the amendments from the AmendmentTable |
There was a problem hiding this comment.
I don't quite follow this comment. If we hit this code, the Retire2017Amendments amendment has gained majority is being activated. This happens in lockstep due to the way consensus operates. The only servers that won't be in lockstep are servers that are about to become amendment blocked anyways.
What am I missing?
There was a problem hiding this comment.
I see your perspective. In a practical sense all validators should be doing this on the same ledger and the outcome should, under ordinary circumstances, be a foregone conclusion.
What I'm attempting to say here is that, in general, we don't assume that all validators move in lock step. We expect them to produce different ledgers initially and then negotiate a ledger they can all agree on. Just because the Change transaction is applied locally doesn't allow us to assume that the Change will be in the validated ledger. We have to wait for all of the validators to weigh in before we can assume that.
Once we see featureRetire2017Amendments in a validated ledger then we're allowed to modify AmendmentTable. But at this point we're not allowed to assume that everyone will agree on featureRetire2017Amendments. So we wait and make the change in AmendmentTable:: doValidatedLedger.
If you find the comment confusing I'm happy to remove it or reword it. Or maybe I'm just confused...
| // yet since the AmendmentTable is not a ledger object. Just | ||
| // because we do the Change doesn't guarantee enough other | ||
| // validators will. | ||
| for (uint256 const& retire : detail::retiringAmendments()) |
There was a problem hiding this comment.
Isn't this code basically std::set_difference?
There was a problem hiding this comment.
Good spotting, and almost. But set_difference operates on sorted ranges and neither detail::retiringAmendments nor amendments are sorted.
There was a problem hiding this comment.
Which raises the interesting question of whether they should be. The existing "temporal" order has one benefit: it shows what activated in what order, but sorting offers benefits too. No change needed; just thinking out loud.
Codecov Report
@@ Coverage Diff @@
## develop #3368 +/- ##
===========================================
+ Coverage 70.41% 70.44% +0.02%
===========================================
Files 683 683
Lines 54775 54806 +31
===========================================
+ Hits 38570 38606 +36
+ Misses 16205 16200 -5
Continue to review full report at Codecov.
|
|
The spec and in code comments are really helpful! To confirm my understanding, all nodes will remove the retired amendments from the ledger at the same ledger sequence. But the fixing of AmendmentTable data structure is done later and nodes may do it at different times, which is ok. |
If the Retire2017Amendments amendment is activated, then those amendments that are ... a. Enabled, b. No longer dependent on runtime amendment checks, and c. Listed in detail::retiringAmendments() ... will be removed from the Amendment ledger object and the AmendmentTable. The motivation for this change is to reduce the number of amendments stored in the Amendment ledger object. It also reduces noise in the feature RPC command (and source code) regarding amendments that have been enabled for a long time.
scottschurr
force-pushed
the
retire-amendments
branch
from
447e423 to
cf6829b
Compare
|
Squashed, clang-formatted, and rebased to 1.6.0-b4. |
|
Having spoken with @scottschurr, I think that we don't quite feel comfortable merging this in yet. The code is fine, but this is a "breaking change" from a user's standpoint. The spec behind this will remain open, to solicit further feedback. Please see XRPLF/XRPL-Standards#14 |
If the Retire2017Amendments amendment is activated, then those amendments that are ...
will be removed from the Amendment ledger object and the AmendmentTable.
The motivation for this change is to reduce the number of amendments stored in the Amendment ledger object. It also reduces noise in the feature RPC command (and source code) regarding amendments that have been enabled for a long time.