Refactor unapplied transaction queue #295
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Refactor incoming trx handling
…actions in progress
…te trx instead of partial trace.
…tion to be notified
Reorder return_failure_trace flag in structure to match method order
…ilure_trace in unapplied trx queue. Add producer plugin test for trxs
Fix merge issues
…end, in unit tests
brianjohnson5972
suggested changes
May 24, 2022
| { trx, expiry, persist_until_expired ? trx_enum_type::incoming_persisted : trx_enum_type::incoming, return_failure_trace, std::move( next ) } ); | ||
| if( insert_itr.second ) added( insert_itr.first ); | ||
| } else { | ||
| if( !(itr->trx_meta == trx) && next ) { |
There was a problem hiding this comment.
Not sure that this logic makes sense, and the comment on the next line seems wrong. I think at the very least we should return whether or not next is defined here (and will we ever not have next set??).
…as to keep it around in case it was aborted, but it would be removed as a duplicate on next speculative block anyway, so it is meaningless to keep it. Simplify add_incoming. add_incoming should never change persist_until_expired or return_failure_trace flags, so just report duplicate. No need to update anything.
brianjohnson5972
approved these changes
May 26, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
unapplied_transaction_queue.unapplied_transaction_queueused to have aprocess_modewhere it would not store aborted or forked-out transactions when it knew that the node would never produce a block. This was an optimization because there seemed to be no reason to cache aborted or forked-out transactions. Since the trx has been validated locally and will not be validated again, why keep it around? If you are producing you need to keep it so on a fork-switch the trxs are not lost. But there seemed to be no reason to keep them for speculative mode. However, theunapplied_transaction_queueis also used when validating a block to look up already validated and constructedtransaction_metadata. Keeping the already validated trxs in the queue allows for faster validation of received blocks for all the transactions already received by the node.last-block-time-offset-uswhich doesn't really make any sense since the block is only speculative and will not be broadcast on the network. With this PR, all speculative blocks use theproduce-time-offset-usincluding the 12th block.Resolves: #205