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 1 commit
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.
if we allow gaps to exist and ordering here doesnt matter, should we look into making nonces expire. A nonce is accompanied with a blockheight or hash at which the nonce will expire.
This leads into the direction of not needing nonces at all. Since we move from caring about order and replay to only replay protection then nonces could be simplified.
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, the basic idea is to allow "unordered" account with as low as possible overhead.
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.
based on the prototype implementation in this ADR, we can record the block height or time the latest nonce is created, and when it timeouts make all the "gaps" expire at the same time, in this way, the overhead should be minimal.
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.
Can you please add more to detail and generally explain the concept of "gaps". I think it's clear if you have context, but it'll be better for the reader.
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.
Added a bit more in decisions sections, please re-review
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.
Consider optimizing this by using a more efficient method to add multiple gaps at once if such functionality is supported by the
IntSet
.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 loop in lines 42-43 may introduce a performance issue if the gap is large. Consider optimizing this by using a more efficient method to add multiple gaps at once if such functionality is supported by the
IntSet
.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
NewIntSet
function should return a pointer toIntSet
.Commitable suggestion
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.
Clarify the comment to reflect the actual behavior.
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.
Are we guaranteed for this nonce/gap to be used and safe to be discarded?
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.
when it's removed from the set, it just means the nonce can't be used anymore, if there's a old pending transaction still using that nonce, it won't be accepted anymore, like an expiration.
so there are two kinds of expiration here, timestamp based, and gap set capacity based.
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 comment on line 73 might be misleading as it implies that the smallest value is always removed, which may not be the case if the smallest value is still a valid gap. Clarify the comment to reflect the actual behavior.
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.
nit: Move this section to the top :)
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.
done