TxQ developer docs, and faster ledger size growth #2682
Conversation
ximinez
requested review from
mellery451 and
scottschurr
and removed request for
mellery451
Jenkins Build SummaryBuilt from this commit Built at 20181002 - 00:32:16 Test Results
|
src/ripple/app/misc/FeeEscalation.md
Outdated
| the validated ledger until it gets to [50](#other-constants), at | ||
| which point, the limit will be the largest number of transactions | ||
| be the max of the number of transactions in the validated ledger | ||
| plus [25%](#other-constants) or the current limit until it gets |
There was a problem hiding this comment.
is this 25% number here the same as normal_consensus_increase_percent? If so, elsewhere the default says 20%.
There was a problem hiding this comment.
Woops. Dang, that messed up the example, too. Fixed.
src/ripple/app/misc/FeeEscalation.md
Outdated
| new largest value by 10% each time the ledger has more than 50 | ||
| transactions. | ||
| more than [50](#other-constants) transactions. Any time the limit | ||
| decreases (ie. a large ledger is no longer recent), the limit will |
src/ripple/app/misc/FeeEscalation.md
Outdated
| drops, but the 22nd transaction will require | ||
| a level of about 355,000 or about 13,800 drops. | ||
| * This will cause the first 26 transactions only require 10 | ||
| drops, but the 27nd transaction will require |
src/ripple/app/misc/FeeEscalation.md
Outdated
| @@ -262,6 +276,7 @@ Result format: | |||
| "current_queue_size" : "2", // number of transactions waiting in the queue | |||
| "expected_ledger_size" : "15", // one less than the number of transactions that can get into the open ledger for the base fee. | |||
| "max_queue_size" : "300", // number of transactions allowed into the queue | |||
| "ledger_current_index" : 123456789, // sequence number of the current open ledger | |||
src/ripple/app/misc/TxQ.h
Outdated
| /* So we don't deal with infinite fee levels, treat | ||
| any transaction with a 0 base fee (ie. SetRegularKey | ||
| /** So we don't deal with "infinite" fee levels, treat | ||
| any transaction with a 0 base fee (ie SetRegularKey |
src/ripple/app/misc/TxQ.h
Outdated
| @@ -267,18 +425,36 @@ class TxQ | |||
| }; | |||
| } | |||
|
|
|||
| /** Use the number of transactions in the current open ledger | |||
| to compute the fee level a transaction must pay bypass the | |||
src/ripple/app/misc/TxQ.h
Outdated
| /// Reference transaction fee level | ||
| std::uint64_t referenceFeeLevel; | ||
| /// Minimum fee level to get in the queue | ||
| std::uint64_t minFeeLevel; |
There was a problem hiding this comment.
Noting the difference between minFeeLevel and expFeeLevel. minFeeLevel is to get into the TxQ. expFeeLevel is to get into the current open ledger. Their names don't reflect that they operate on fundamentally different stuff (the TxQ vs the open ledger). I think that could be an added point of confusion in an already confusing area.
I don't mind long names, so I'll suggest txQMinFeeLevel and estOpenLedgerFeeLevel. I know that others are allergic to long names, so I don't necessarily expect you to use those.
There was a problem hiding this comment.
The thinking behind those names, IIRC, is that minFeeLevel is the minimum level to even be considered, while expFeeLevel is the expected level to be processed immediately.
What do you think about minProcessingFeeLevel and openLedgerFeeLevel? I think I'll also update the documentation again:
minProcessingFeeLevel: minimum fee level for a transaction to be considered for the open ledger or the queueopenLedgerFeeLevel: minimum fee level to get into the current open ledger, bypassing the queue
src/ripple/app/misc/TxQ.h
Outdated
| boost::optional<TxConsequences const> consequences; | ||
| }; | ||
|
|
||
| /** | ||
| Structure returned by @ref TxQ::getTxs to describe | ||
| all transactions in the queue for the open ledger. |
There was a problem hiding this comment.
This description doesn't look right. The description talks about "all transactions". But the data structure contains a single shared_ptr to a single transaction. Does getTxs() return one of these for each transaction in the queue? If so, then a better comment might be...
"Structure that describes one transaction that should be considered for the open ledger."
There was a problem hiding this comment.
Looking at this description again, the phrasing is super-awkward. getTxs is what returns the descriptions for all transactions... I'm going to rewrite this one.
| /** The *intermediate* result returned by @ref preflight before | ||
| this transaction was queued. This will always be | ||
| `tesSUCCESS`. | ||
| */ |
There was a problem hiding this comment.
Hmmm. Interesting. If preflightResult is always tesSUCCESS, then what's the point in storing the value? Or even asking about it?
There was a problem hiding this comment.
IIRC, when I initially created the field, I was concerned there might be cases where it had some other value. Once I was convinced that there weren't it didn't occur to me to remove it. I'll double check, and do that now.
This description is wrong. There is an edge case where the result may not be tesSUCCESS - specifically if the rules or flags change after a transaction is queued, then TxQ::MaybeTx::apply can preflight again. It's unlikely, but I believe it's possible for that process to return a code that is not tesSUCCESS, but does not cause the transaction to be immediately removed from the queue, particularly if the attempt was made by TxQ::tryClearAccountQueue (speaking of complexity...).
There was a problem hiding this comment.
Thanks for checking. You'll be adding a comment about the corner case I assume...
src/ripple/app/misc/TxQ.h
Outdated
| Computes the total fee level for all transactions in a series. | ||
| Assumes that there are already more than @ref txnsExpected_ txns | ||
| in the view. If there aren't, the math still works, but the level | ||
| will be higher than actually required. |
There was a problem hiding this comment.
Does the "the math still works" mean that the math doesn't crash? The phrase, "the level will be higher than actually required" suggests that the math doesn't really work...
There was a problem hiding this comment.
"The math still works" means you're not going to get overflows, underflows, imaginary numbers, sqrt(pi), etc. I've rephrased the comment.
src/ripple/app/misc/TxQ.h
Outdated
| @param seriesSize Total number of transactions in the series to be | ||
| processed. | ||
|
|
||
| @return A `std::pair` as returned from `mulDiv` indicating whether |
There was a problem hiding this comment.
What are the conditions when the returned bool might be false? Perhaps this would be a good use for a boost::optional>?
There was a problem hiding this comment.
That's why I reference mulDiv. https://github.com/ximinez/rippled/blob/txq_doc/src/ripple/basics/mulDiv.h#L29
The bool can be false if the calculation overflows. The reason it returns a pair instead of an optional is that it populates a maxint value for the result, which the caller can use anyways if it wants.
There was a problem hiding this comment.
Hmmm. Fair. If I'm desperate enough I can go look up the documentation on mulDiv at the cost of some small additional annoyance 😉 .
There was a problem hiding this comment.
I did update the description slightly, to mention that the flag indicates overflow.
| @@ -287,34 +440,65 @@ class TxQ | |||
| std::size_t extraCount, std::size_t seriesSize); | |||
| }; | |||
|
|
|||
| /** | |||
| Represents transactions in the queue which may be applied | |||
| later to the open ledger. | |||
There was a problem hiding this comment.
Are there transactions in the queue that may never be applied to an open ledger? If so, why?
There was a problem hiding this comment.
There are several ways a transaction could fail to apply. Item 4 at https://github.com/ximinez/rippled/blob/txq_doc/src/ripple/app/misc/FeeEscalation.md#transaction-queue explains the possible results for a transaction in the queue.
There was a problem hiding this comment.
Fair. I think what you're saying is that all transactions in the queue are intended to be applied to an open ledger (where the transaction may fail). Any given transaction in the queue may expire before that attempt is made. But there are no transactions in the queue which we know we will never apply to an open ledger.
And, no, I'm not going to ask you to put that in the comments 😉
There was a problem hiding this comment.
Correct. In fact, the biggest qualification for whether a transaction can be put into the queue is whether it is "likely to successfully claim a fee". And that is in the docs somewhere. 😀
| TxSeq const sequence; | ||
| /// Flags provided to `apply`. If the transaction is later | ||
| /// attempted with different flags, it will need to be | ||
| /// `preflight`ed again. |
There was a problem hiding this comment.
What would cause the flags to change?
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks. Sorry, I wasn't thinking about the flags in a broad enough sense. The comment is good as is.
| on the current metrics. If uninitialized, there is not limit, | ||
| but that condition can not last for long in practice. | ||
| @note This member must always and only be accessed under | ||
| locked mutex_ |
There was a problem hiding this comment.
Two suggestions:
- "... there is no limit ..."
- "... condition cannot last ..."
There was a problem hiding this comment.
Generally looks good. I mostly had nits and questions.
I think my biggest concern is that I'd like you to convince me why this doesn't need to be on an amendment. I agree that this change doesn't directly affect any individual transaction. But it influences which transactions are considered for the open ledger. So at the point where this change is partially installed on the network different servers will build different ledgers. I think this could cause desyncing. And the validators might have more trouble coming to consensus during the transaction period. Putting this change on amendment would stabilize the transition.
I'm open to being convinced otherwise.
src/ripple/app/misc/FeeEscalation.md
Outdated
| * This will cause the first 21 transactions only require 10 | ||
| drops, but the 22nd transaction will require | ||
| a level of about 355,000 or about 13,800 drops. | ||
| * This will cause the first 26 transactions only require 10 |
There was a problem hiding this comment.
"... transactions to only ..."?
src/ripple/app/misc/FeeEscalation.md
Outdated
| @@ -100,7 +107,7 @@ ledger from highest [fee level](#fee-level) to lowest. These transactions | |||
| count against the open ledger limit, so the required [fee level](#fee-level) | |||
| may start rising during this process. | |||
| 3. Once the queue is empty, or required the [fee level](#fee-level) | |||
There was a problem hiding this comment.
"... or required the ..." -> "... or the required ..."?
src/ripple/app/misc/FeeEscalation.md
Outdated
| value of 20% was chosen to allow the limit to grow quickly as load | ||
| increases, but not so quickly as to allow bad actors to run unrestricted. | ||
| The reduction value of 50% was chosen to cause the limit to drop | ||
| significantly, but not so drastically that the limit can not quickly |
There was a problem hiding this comment.
Micro nit: you might look around these files for instances of "can not" and consider using "cannot" instead. I believe that "cannot" is, under most circumstances, considered the more desirable spelling.
There was a problem hiding this comment.
I would swear that I remember from my high school grammar rules that "cannot" is not only less preferred, but not legit at all. But I looked around a little bit, and yeah, I'm just wrong.
Codecov Report
@@ Coverage Diff @@
## develop #2682 +/- ##
===========================================
- Coverage 70.02% 70.01% -0.01%
===========================================
Files 703 704 +1
Lines 55251 55272 +21
===========================================
+ Hits 38687 38698 +11
- Misses 16564 16574 +10
Continue to review full report at Codecov.
|
The biggest reason I can give is precedent, including another PR that used a different "grow quickly, shrink slowly" method (#2235), one from just a few months ago that affected when transactions could come out of the ledger (#2567), the one that allowed clearing those "series" we discussed earlier (#1800), and the one that added support for multiple transactions per account (#1639). But precedent is not conclusive. These are my other reasons why I think this doesn't need an amendment:
|
|
As discussed with @scottschurr this morning, the biggest reason that this change doesn't need an amendment is that it doesn't affect the way transactions are processed through consensus. Even if consensus builds a ledger that includes transactions that a given node doesn't think paid enough, it will still include those transactions in it's validated ledger before continuing on. And since the transactions are just as likely to be available locally, the node is no more or less likely to desynchronize from the network. I'm going to go ahead and mark this PR as passed. If there are any objections, I'll undo it. |
ximinez
added
the
Passed
|
Please squash this down. |
ximinez
force-pushed
the
txq_doc
branch
2 times, most recently
from
ce450f2 to
0d15a04
Compare
|
Squashed and rebased onto 1.1.0. |
* Rename a couple of member variables for clarity.
* When increasing the expected ledger size, add on an extra 20%. * When decreasing the expected ledger size, take the minimum of the validated ledger size or the old expected size, and subract another 50%. * Update fee escalation documentation. * Refactor the FeeMetrics object to use values from Setup * Resolves RIPD-1630.
Two separate commits, I'm only putting them together because they both affect the
TxQ.TxQand related classes. I think I got every class, function, and member.Note: I don't believe this change requires an amendment because it does not affect transaction processing, only queuing behavior.