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 16 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.
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.