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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
adr: Un-Ordered Transaction Inclusion #18553
adr: Un-Ordered Transaction Inclusion #18553
Changes from 19 commits
64d664f
998f092
6613f1a
345987c
50db7fe
0ec3426
ebef016
d95720a
9e704f8
aad93e2
d62abcd
8ef0158
8ab2537
8110ed5
2c32a75
a2df33e
14bb094
2eafdbb
174cb3b
1dc13ed
944760c
3aa46f3
71abaa1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The abstract contains a grammatical error. Consider revising to "it doesn't use nonces at all" for clarity and correctness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A short
timeout_height
window also ensures a tighter bound on replay protection.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
unordered
field in theTxBody
message is incorrectly defined asboolean
. The correct Protobuf type isbool
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a syntax error in the
Size
method of theDedupTxHashManager
struct. It should belen(dtm.hashes)
instead oflen(dtm).hashes
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how will this run in endblock? could we not do a quick check against current time and remove everything behind it? could be a
DeleteUntil
then it checks and removes all previous hashes. This could be done async as well if we wantedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wire into baseapp.
Yeah, it can run in background, the end blocker could be a trigger.
It could grab a read lock first and iterate to find out expired hashes, then grab write lock to do the deletion, that should have maximum parallel performance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that sounds good. Since everything under current height wont be touched by the state machine, do we need locks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need locks because check tx runs concurrently, and we also have a background purging loop now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
AnteHandle
method inDedupTxDecorator
should check for duplicates before adding a new transaction hash to the dictionary, even when!ctx.IsCheckTx()
is true.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could simply purge the map during the
FinalizeBlock
hook that app's can set. There are many places we can purge, it can beEndBlock
too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe
Commit
event, I changed the method name toOnNewBlock
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, overall, my point is that I think this is a minor implementation detail in the grand scheme of things 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use the filesystem to write the known hashes at shutdown? On start we would populate from the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as we write to disk, we need to handle commit, rollbacks the same as the other state machine state, to keep it consistent. So better to use existing non-iavl kvstore directly.
but we can cache the whole thing in memory on start up, so duplication checking is fast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do something like this in store, but not sure its exposed to things out side of store currently. The main worry i have is around upgrades, where everyone stops and starts, then the map would be empty. Looking back at the x blocks would be best but since we dont know if everyone has it, we could end up in a bad situation.
we could create a simple txhash store for store v1 that handles this and is exposed through baseapp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, we need some support from storage layer here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The consequences section should include the negative aspect of the nonce max number becoming 2^63, as mentioned in the existing comments.