Skip to content
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

Merged
merged 23 commits into from
Dec 5, 2023

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Nov 24, 2023

Description

replay-attack protection without using nonce.

rendered

ref: #13009


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • run make lint and make test
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

Summary by CodeRabbit

Summary by CodeRabbit

  • Documentation
    • Introduced ADR 069, detailing the concept of an unordered account to enhance concurrent transaction processing.
    • Proposed modifications to account nonce logic to support gaps, improving transaction inclusion flexibility.
    • Described the introduction of a new nonce "lane" for optional unordered transaction inclusion, benefiting use cases like IBC relayers and crypto exchanges.
    • Outlined the implementation of a roaring bitmap for efficient gap value management.
    • Explained the impact on user experience, including support for concurrent transactions and minimal overhead when using the new feature.

@yihuang yihuang requested a review from a team as a code owner November 24, 2023 07:09
Copy link
Contributor

coderabbitai bot commented Nov 24, 2023

Walkthrough

ADR 069 proposes the introduction of an "un-ordered nonce lane" to support concurrent transaction inclusion, allowing gaps in nonce sequences. This feature aims to enhance the user experience for sending multiple transactions simultaneously, particularly for use cases such as IBC relayers and crypto exchanges. The proposal encompasses changes to the account structure, transaction format, and nonce management, utilizing a roaring bitmap for tracking gaps. Additionally, it introduces a new decorator for duplicate prevention and transaction handling, aiming to maintain nonce uniqueness with minimal overhead.

Changes

File Path Change Summary
docs/architecture/adr-069-unordered-account.md Introduced ADR 069, proposing an un-ordered account concept with a new Gaps field in the Account struct, an IntSet type for managing gaps, and modifications to the CheckNonce function.
docs/architecture/adr-069-unordered-account.md Added an extra nonce "lane" for concurrent transaction inclusion, repurposing the high bit of the nonce for lane identification, and extending the account state with an unorderedNonce field.
docs/architecture/adr-069-unordered-account.md Introduced an UnorderedNonceManager to manage un-ordered nonce values and a boolean field unordered in the BaseAccount message, with nonce validation and expiration logic.
docs/architecture/adr-069-unordered-account.md Added a boolean field unordered to the TxBody message, a DedupTxDecorator for handling un-ordered transactions, and a DedupTxHashManager for duplicate checking and expiration.

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 875a71c and 64d664f.
Files selected for processing (1)
  • docs/architecture/adr-069-unordered-account.md (1 hunks)
Additional comments: 2
docs/architecture/adr-069-unordered-account.md (2)
  • 26-30:
    The addition of the Gaps field to the Account struct aligns with the proposed changes for supporting gaps in nonce values.

  • 56-87:
    The IntSet implementation with methods Add, Remove, and Contains is consistent with the requirements for managing nonce gaps.

Comment on lines 33 to 53
func(acc *Account) CheckNonce(nonce int64) error {
switch {
case nonce == acct.SequenceNumber:
return errors.New("nonce is occupied")
case nonce >= acct.SequenceNumber + 1:
gaps := nonce - acct.SequenceNumber - 1
if gaps > MaxGap {
return errors.New("max gap is exceeded")
}
for i := 0; i < gaps; i++ {
acct.Gaps.Add(i + acct.SequenceNumber + 1)
}
acct.SequenceNumber = nonce
case nonce < acct.SequenceNumber:
if !acct.Gaps.Contains(nonce) {
return errors.New("nonce is occupied")
}
acct.Gaps.Remove(nonce)
}
return nil
}
Copy link
Contributor

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.

Comment on lines 71 to 78
func (is *IntSet) Add(n int) {
if is.bitmap.Length() >= is.capacity {
// pop the minimal one
is.Remove(is.bitmap.Minimal())
}

is.bitmap.Add(n)
}
Copy link
Contributor

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 64d664f and 998f092.
Files selected for processing (1)
  • docs/architecture/adr-069-unordered-account.md (1 hunks)

Comment on lines 64 to 68
func NewIntSet(capacity int) *IntSet {
return IntSet{
capacity: capacity,
bitmap: *roaringbitmap.New(),
}
Copy link
Contributor

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 to IntSet.

-  return IntSet{
+  return &IntSet{

Commitable suggestion

IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
func NewIntSet(capacity int) *IntSet {
return IntSet{
capacity: capacity,
bitmap: *roaringbitmap.New(),
}
func NewIntSet(capacity int) *IntSet {
return &IntSet{
capacity: capacity,
bitmap: *roaringbitmap.New(),
}

Comment on lines 42 to 43
for i := 0; i < gaps; i++ {
acct.Gaps.Add(i + acct.SequenceNumber + 1)
Copy link
Contributor

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.

Comment on lines 73 to 74
// pop the minimal one
is.Remove(is.bitmap.Minimal())
Copy link
Contributor

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.

@lasarojc
Copy link

This is a very interesting ADR and aligns very well with CometBFT vision and as such we would like to contribute.

There have been several discussions about the pros and cons of using the mempool provided by CometBFT versus an app-side one, being the most recent in the context of ADR 111 The CometBFT teams believes that the end-to-end principle applies to the mempool and that application is best prepared implement it.

Specially wrt ordering of transactions, even if CometBFT does not provide FIFO ordering guarantees (for example as discussed here), FIFO is frequently achieved and applications may have come to depend on it for performance, if not for correctness. If this ADR results in a lesser expectation of ordering from CometBFT, we could, for example, review the decision not to merge the changes made in the context of issue 1149, which provides a simple non-breaking change to saving bandwidth but which cause more frequent violations to FIFO.

Regarding this ADR, some questions, which probably stem from my lacking of understanding of the SDK:

  • Is the idea to use gaps to delay reaping transactions (those after gaps) or to somehow include them in the blocks but delay their execution until gaps are filled and decided? I got confused because the context states that "can relax the orderness requirement without lose the uniqueness, in that way we can improve the user experience of concurrent transaction sending" but if the gaps are used to avoid reaping gap-preceedded transactions, order would still be preserved.
  • If a gap nonce is removed due to capacity, not because it was filled, could it happen that at some nodes the transaction with such a nonce is not accepted (return errors.New("nonce is occupied")) but due to asynchronism the transaction is accepted on other nodes? If the accepting nodes are not validators, how will these transactions ever leave the mempool?

cc: @ancazamfir

@yihuang
Copy link
Collaborator Author

yihuang commented Nov 24, 2023

Is the idea to use gaps to delay reaping transactions (those after gaps) or to somehow include them in the blocks but delay their execution until gaps are filled and decided?

Actually the proposal is to allow including transactions into block and execute them out of order. So it's about state machine change, not just mempool.

@lasarojc
Copy link

Actually the proposal is to allow including transactions into block and execute them out of order. So it's about state machine change, not just mempool.

I see. In this case the problem I pointed wouldn't exist, since nodes will close the gaps in the same way.
But in this case, is it the idea that transaction will carry information about the previous one on which it depends, so if the previous one is not executed then the transaction itself will not be executed? If that's the case, then instead of a list of transactions, blocks become a graph (or a forest) of transactions, right?


## Decision

Change the nonce logic to allow gaps to exist, which can be filled by other transactions later, or never filled at all, the prototype implementation use a bitmap to record these gap values.
Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we look into making nonces expire

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.

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be good to understand alternative ways of doing ordering of transactions. I do see people wanting this feature as well.

@yihuang
Copy link
Collaborator Author

yihuang commented Nov 27, 2023

Actually the proposal is to allow including transactions into block and execute them out of order. So it's about state machine change, not just mempool.

I see. In this case the problem I pointed wouldn't exist, since nodes will close the gaps in the same way. But in this case, is it the idea that transaction will carry information about the previous one on which it depends, so if the previous one is not executed then the transaction itself will not be executed? If that's the case, then instead of a list of transactions, blocks become a graph (or a forest) of transactions, right?

no, simply include and execute transactions out of order, they are still executed as the order as in the block.

@tac0turtle
Copy link
Member

Talked with @yihuang in slack. I propose we do two things.

First we keep the current design of nonces, ordering is important for some applications. Outright removing them would cause the need for ordering to happen elsewhere.

Second, we introduce the notion of unordered nonces. This could be under a nonce namespace, in which we have an identifier appended to the nonce to signify there is no need to order the tx with the others.

This keeps transaction ordering as a first class citizen like we have it now, but then we introduce unordered nonces. The main question for unordered nonces, for me, is if we can make them stateless while keeping replay protection.

Comment on lines 73 to 74
// pop the minimal one
is.Remove(is.bitmap.Minimal())
Copy link
Contributor

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?

Copy link
Collaborator Author

@yihuang yihuang Nov 28, 2023

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.

docs/architecture/adr-069-unordered-account.md Outdated Show resolved Hide resolved

## Decision

Change the nonce logic to allow gaps to exist, which can be filled by other transactions later, or never filled at all, the prototype implementation use a bitmap to record these gap values.
Copy link
Contributor

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.

Copy link
Collaborator Author

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

docs/architecture/adr-069-unordered-account.md Outdated Show resolved Hide resolved
docs/architecture/adr-069-unordered-account.md Outdated Show resolved Hide resolved
Comment on lines 89 to 91
## Status

Proposed.
Copy link
Contributor

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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@alexanderbez
Copy link
Contributor

Agree with @tac0turtle -- we need to provide an account with the ability to use standard ordering and concurrent ordering at the same time, in a graceful and clean way, i.e. allow the user to pick.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 998f092 and 6613f1a.
Files selected for processing (1)
  • docs/architecture/adr-069-unordered-account.md (1 hunks)
Additional comments: 1
docs/architecture/adr-069-unordered-account.md (1)
  • 93-97: The NewIntSet function should return a pointer to IntSet.
-  return IntSet{
+  return &IntSet{

docs/architecture/adr-069-unordered-account.md Outdated Show resolved Hide resolved
docs/architecture/adr-069-unordered-account.md Outdated Show resolved Hide resolved
@yihuang yihuang changed the title adr: Un-Ordered Account(support concurrent transactions inclusion) adr: Un-Ordered Nonce Lane(support concurrent transactions inclusion) Nov 28, 2023
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 6613f1a and 345987c.
Files selected for processing (1)
  • docs/architecture/adr-069-unordered-account.md (1 hunks)

docs/architecture/adr-069-unordered-account.md Outdated Show resolved Hide resolved
docs/architecture/adr-069-unordered-account.md Outdated Show resolved Hide resolved
yihuang and others added 7 commits November 28, 2023 09:45
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 345987c and d62abcd.
Files selected for processing (1)
  • docs/architecture/adr-069-unordered-account.md (1 hunks)
Additional comments: 1
docs/architecture/adr-069-unordered-account.md (1)
  • 99-131: The previous reviewer requested more detailed godocs for the CheckNonce function to clarify the logic of each case. It appears that this has not been addressed in the current hunk. Please consider adding more detailed comments to explain the logic, especially for the last two cases.

docs/architecture/adr-069-unordered-account.md Outdated Show resolved Hide resolved
Comment on lines 147 to 154
func (is *IntSet) Add(n uint64) {
if is.bitmap.GetCardinality() >= is.capacity {
// drop the smallest one
is.bitmap.Remove(is.bitmap.Minimal())
}

is.bitmap.Add(n)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the AddRange method, the Add method in the IntSet struct could be optimized. When the capacity is reached and a new element is to be added, the method removes the smallest element one by one. Consider optimizing this logic to handle the removal of elements more efficiently.

Comment on lines 179 to 192
## Consequences

### Positive

* Support concurrent transaction inclusion.
* Only optional fields are added to account state, no state migration is needed.
* No runtime overhead when the new feature is not used.
* No need to change transaction format.

### Negative

- Some runtime overhead when the new feature is used.

## References
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the document outlines the positive and negative consequences of the proposed changes, it may be beneficial to also discuss any potential security implications of introducing an unordered nonce lane, such as the risk of double-spending or other attack vectors.

@yihuang
Copy link
Collaborator Author

yihuang commented Nov 28, 2023

Talked with @yihuang in slack. I propose we do two things.

First we keep the current design of nonces, ordering is important for some applications. Outright removing them would cause the need for ordering to happen elsewhere.

Second, we introduce the notion of unordered nonces. This could be under a nonce namespace, in which we have an identifier appended to the nonce to signify there is no need to order the tx with the others.

This keeps transaction ordering as a first class citizen like we have it now, but then we introduce unordered nonces. The main question for unordered nonces, for me, is if we can make them stateless while keeping replay protection.

Updated the ADR based on the discussion, now we create a new lane for un-ordered nonces, so we'll have both ordered and un-ordered lane runs at the same time, and it has minimal impact to existing user who don't use the unordered feature, please review again, @tac0turtle @lasarojc @alexanderbez

One open question is: should we charge extra fee for extra overhead when user do use the un-ordered lane.

@yihuang yihuang changed the title adr: Un-Ordered Nonce Lane(support concurrent transactions inclusion) adr: Un-Ordered Transaction Inclusion(replay-attack protection without using nonce) Dec 4, 2023
@yihuang yihuang changed the title adr: Un-Ordered Transaction Inclusion(replay-attack protection without using nonce) adr: Un-Ordered Transaction Inclusion Dec 4, 2023
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 2c32a75 and a2df33e.
Files selected for processing (1)
  • docs/architecture/adr-069-unordered-account.md (1 hunks)
Additional comments: 5
docs/architecture/adr-069-unordered-account.md (5)
  • 1-1: The title of the ADR is inconsistent with the PR objective and the summary provided. It should be updated to reflect the correct ADR number and the feature being introduced.
- # ADR 070: Un-Ordered Transaction Inclusion
+ # ADR 069: Un-Ordered Nonce Lane
  • 13-13: The abstract incorrectly states that the proposed method doesn't use nonce at all, which contradicts the PR objective and the summary that mention the introduction of an unordered nonce feature.
- We propose a way to do replay-attack protection without enforcing the order of transactions, it don't use nonce at all.
+ We propose a way to do replay-attack protection without enforcing the order of transactions, by introducing unordered nonces.
  • 21-23: The decision to add a boolean field unordered to the transaction body aligns with the comments from the previous review and the PR objective.

  • 31-36: The protobuf definition for TxBody correctly includes the unordered boolean field as per the decision section.

  • 39-51: The implementation of the ante handlers for bypassing the nonce decorator for un-ordered transactions is consistent with the PR objective and the summary.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between a2df33e and 2eafdbb.
Files selected for processing (1)
  • docs/architecture/adr-069-unordered-account.md (1 hunks)
Additional comments: 2
docs/architecture/adr-069-unordered-account.md (2)
  • 1-1: The title of the ADR in the document is "ADR 070," which is inconsistent with the PR objective and summary that refer to "ADR 069." Please verify the correct ADR number.

  • 13-13: The abstract states that the proposed method doesn't use nonce at all, which contradicts the PR objective and summary that mention introducing an unordered nonce feature. Please clarify whether nonces are used in any capacity.

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, left a few questions in regards to startup. Not sure if replaying last x blocks makes a lot of sense. Its nondeterministic if the blocks are held

}

// EndBlock remove expired tx hashes, need to wire in abci cycles.
func (dtm *DedupTxHashManager) EndBlock(ctx sdk.Context) {
Copy link
Member

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 wanted

Copy link
Collaborator Author

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?

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

Copy link
Member

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?

Copy link
Collaborator Author

@yihuang yihuang Dec 5, 2023

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?

we need locks because check tx runs concurrently, and we also have a background purging loop now.


### Negative

- Start up overhead to scan historical blocks.
Copy link
Member

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

Copy link
Collaborator Author

@yihuang yihuang Dec 4, 2023

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.

Copy link
Member

@tac0turtle tac0turtle Dec 4, 2023

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.

Copy link
Collaborator Author

@yihuang yihuang Dec 5, 2023

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.

@tac0turtle
Copy link
Member

discussed on the team call, should we say 0 nonce = unordered? we can do this for backports, should we do this instead of the boolean as well? cc @alexanderbez

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConceptACK.

docs/architecture/adr-069-unordered-account.md Outdated Show resolved Hide resolved
docs/architecture/adr-069-unordered-account.md Outdated Show resolved Hide resolved

When an un-ordered transaction are included into block, the transaction hash is recorded in a dictionary, new transactions are checked against this dictionary for duplicates, and to prevent the dictionary grow indefinitly, the transaction must specify `timeout_height` for expiration, so it's safe to removed it from the dictionary after it's expired.

The dictionary can be simply implemented as an in-memory golang map, a preliminary analysis shows that the memory consumption won't be too big, for example `32M = 32 * 1024 * 1024` can support 1024 blocks where each block contains 1024 unordered transactions. For safty, we should limit the range of `timeout_height` to prevent very long expiration, and limit the size of the dictionary.
Copy link
Contributor

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.

Comment on lines 155 to 157
### EndBlocker

Wire up the `EndBlock` method of `DedupTxHashManager` into the application's abci life cycle.
Copy link
Contributor

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 be EndBlock too.

Copy link
Collaborator Author

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 to OnNewBlock.

Copy link
Contributor

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 👍

@yihuang
Copy link
Collaborator Author

yihuang commented Dec 4, 2023

discussed on the team call, should we say 0 nonce = unordered? we can do this for backports, should we do this instead of the boolean as well? cc @alexanderbez

Although it don't change tx format, but it changes the behavior, there's a slight chance that an invalid tx before becomes valid now because of this change, I think we shouldn't backport this.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 4

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 2eafdbb and 174cb3b.
Files selected for processing (1)
  • docs/architecture/adr-069-unordered-account.md (1 hunks)
Additional comments: 4
docs/architecture/adr-069-unordered-account.md (4)
  • 1-1: The ADR title in the document is "ADR 070", which is inconsistent with the summary that refers to "ADR 069". Please verify the correct ADR number.

  • 53-90: The EndBlock method and other methods in DedupTxHashManager use a mutex, but the mutex is not declared within the provided code. Ensure that the mutex is properly declared and initialized.

  • 98-104: The AnteHandle method in IncrementSequenceDecorator correctly bypasses the nonce decorator for un-ordered transactions.

  • 165-173: The consequences section is concise and mentions the negative impact of start-up overhead. No further issues are identified based on the previous instructions to ignore old comments.


## Abstract

We propose a way to do replay-attack protection without enforcing the order of transactions, it don't use nonce at all. In this way we can support un-ordered transaction inclusion.
Copy link
Contributor

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 4

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 174cb3b and 1dc13ed.
Files selected for processing (1)
  • docs/architecture/adr-069-unordered-account.md (1 hunks)
Additional comments: 1
docs/architecture/adr-069-unordered-account.md (1)
  • 1-1: The ADR title is inconsistent with the PR title and the summary provided. The PR title and summary mention ADR 069, but the ADR document itself is titled ADR 070.

message TxBody {
...

boolean unordered = 4;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unordered field in the TxBody message is incorrectly defined as boolean. The correct Protobuf type is bool.

dtm.mutex.RLock()
defer dtm.mutex.RUnlock()

return len(dtm).hashes
Copy link
Contributor

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 the DedupTxHashManager struct. It should be len(dtm.hashes) instead of len(dtm).hashes.

Comment on lines 138 to 149
if !ctx.IsCheckTx() {
// a new tx included in the block, add the hash to the dictionary
if dtd.m.Size() >= MaxNumberOfTxHash {
return nil, errorsmod.Wrap(sdkerrors.ErrLogic, "dedup map is full")
}
dtd.m.Add(tx.Hash(), tx.TimeoutHeight())
} else {
// check for duplicates
if dtd.m.Contains(tx.Hash()) {
return nil, errorsmod.Wrap(sdkerrors.ErrLogic, "tx is duplicated")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AnteHandle method in DedupTxDecorator should check for duplicates before adding a new transaction hash to the dictionary, even when !ctx.IsCheckTx() is true.


### Negative

- Start up overhead to scan historical blocks.
Copy link
Contributor

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.

@alexanderbez alexanderbez added this pull request to the merge queue Dec 5, 2023
Merged via the queue into cosmos:main with commit 623cdf0 Dec 5, 2023
49 of 53 checks passed
@yihuang yihuang deleted the unordered-account branch December 6, 2023 01:02
marcello33 added a commit to 0xPolygon/cosmos-sdk that referenced this pull request Dec 13, 2023
* feat: secp256k1 public key constant time (cosmos#18026)

Signed-off-by: bizk <[email protected]>

* chore: Fixed changelog duplicated items (cosmos#18628)

* adr: Un-Ordered Transaction Inclusion (cosmos#18553)

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>

* docs: lint ADR-070 (cosmos#18634)

* fix(baseapp)!: postHandler should run regardless of result (cosmos#18627)

* docs: fix typos in adr-007-specialization-groups.md (cosmos#18635)

* chore: alphabetize labels (cosmos#18640)

* docs(x/circuit): add note on ante handler (cosmos#18637)

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

* fix: telemetry metric label variable (cosmos#18643)

* chore: typos fix (cosmos#18642)

* refactor(store/v2): updates from integration (cosmos#18633)

* build(deps): Bump actions/setup-go from 4 to 5 (cosmos#18647)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

* feat(store/v2): snapshot manager (cosmos#18458)

* chore(client/v2): fix typos in the README.md (cosmos#18657)

* fix(baseapp):  protocompat.go gogoproto.Merge does not work with custom types (cosmos#18654)

Co-authored-by: unknown unknown <unknown@unknown>

* chore: fix several minor typos (cosmos#18660)

* chore(tools/confix/cmd): fix typo in view.go (cosmos#18659)

* refactor(x/staking): check duplicate addresses in StakeAuthorization's params (cosmos#18655)

* feat(accounts): use gogoproto API instead of protov2.  (cosmos#18653)

Co-authored-by: unknown unknown <unknown@unknown>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* fix(store/commitment/iavl): honor tree.Remove error firstly (cosmos#18651)

* build(deps): Bump actions/stale from 8 to 9 (cosmos#18656)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(docs): fix typos & wording in docs (cosmos#18667)

* chore: fix several typos.   (cosmos#18666)

* feat(telemetry): enable `statsd` and `dogstatsd` telemetry sinks (cosmos#18646)

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: marbar3778 <[email protected]>
Co-authored-by: Marko <[email protected]>

* feat(store/v2): add SetInitialVersion in SC (cosmos#18665)

* feat(client/keys): support display discreetly for `keys add` (cosmos#18663)

Co-authored-by: Julien Robert <[email protected]>

* ci: add misspell action (cosmos#18671)

* chore: typos fix by misspell-fixer (cosmos#18683)

Co-authored-by: github-merge-queue <[email protected]>
Co-authored-by: Julien Robert <[email protected]>

* chore: add v0.50.2 changelog to main (cosmos#18682)

* build(deps): Bump github.com/jhump/protoreflect from 1.15.3 to 1.15.4 in /tests (cosmos#18678)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* refactor(bank): remove .String() calls  (cosmos#18175)

Co-authored-by: Facundo <[email protected]>

* ci: use codespell instead of misspell-fixer (cosmos#18686)

Co-authored-by: Marko <[email protected]>

* feat(gov): add proposal types and spam votes (cosmos#18532)

* feat(accounts): use account number as state prefix for account state (cosmos#18664)

Co-authored-by: unknown unknown <unknown@unknown>

* chore: typos fixes by cosmos-sdk bot (cosmos#18689)

Co-authored-by: github-merge-queue <[email protected]>
Co-authored-by: Julien Robert <[email protected]>
Co-authored-by: marbar3778 <[email protected]>

* feat(client/keys): support display discreetly for keys mnemonic (cosmos#18688)

* refactor: remove panic usage in keeper methods (cosmos#18636)

* ci: rename pr name in misspell job (cosmos#18693)

Co-authored-by: Marko <[email protected]>

* build(deps): Bump github.com/pelletier/go-toml/v2 from 2.1.0 to 2.1.1 in /tools/confix (cosmos#18702)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

* feat(client/keys): support display discreetly for keys export (cosmos#18684)

* feat(x/gov): better gov genesis validation (cosmos#18707)

---------

Signed-off-by: bizk <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Carlos Santiago Yanzon <[email protected]>
Co-authored-by: yihuang <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Co-authored-by: Facundo Medica <[email protected]>
Co-authored-by: Akaonetwo <[email protected]>
Co-authored-by: Marko <[email protected]>
Co-authored-by: Julien Robert <[email protected]>
Co-authored-by: dreamweaverxyz <[email protected]>
Co-authored-by: Pioua <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: cool-developer <[email protected]>
Co-authored-by: leonarddt05 <[email protected]>
Co-authored-by: testinginprod <[email protected]>
Co-authored-by: unknown unknown <unknown@unknown>
Co-authored-by: Sukey <[email protected]>
Co-authored-by: axie <[email protected]>
Co-authored-by: Luke Ma <[email protected]>
Co-authored-by: Emmanuel T Odeke <[email protected]>
Co-authored-by: 0xn4de <[email protected]>
Co-authored-by: hattizai <[email protected]>
Co-authored-by: Devon Bear <[email protected]>
Co-authored-by: Marko <[email protected]>
Co-authored-by: Halimao <[email protected]>
Co-authored-by: Cosmos SDK <[email protected]>
Co-authored-by: github-merge-queue <[email protected]>
Co-authored-by: Facundo <[email protected]>
Co-authored-by: Likhita Polavarapu <[email protected]>
@marcello33 marcello33 mentioned this pull request Jan 22, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants