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
Merged
Changes from 2 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 106 additions & 0 deletions docs/architecture/adr-069-unordered-account.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
# ADR 069: Un-Ordered Account

## Changelog

* Nov 24, 2023: Initial Draft

## Abstract

Support gaps in account nonce values to support un-ordered(concurrent) transaction inclusion.

## Context

Right now the transactions from a particular sender must be included in strict order because of the nonce value requirement, which makes it tricky to send many transactions concurrently, for example, when a previous pending transaction fails or timeouts, all the following transactions are all blocked. Relayer and exchanges are some typical examples.

The main purpose of nonce value is to protect against replay attack, but for that purpose we only need to make sure the nonce is unique, so we can relax the orderness requirement without lose the uniqueness, in that way we can improve the user experience of concurrent transaction sending.
yihuang marked this conversation as resolved.
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
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
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


It's debatable how we should configure the user accounts, for example, should we change the default behavior directly or let user to turn this feature on explicitly, or should we allow user to set different gap capacity for different accounts.

```golang
const MaxGap = 1024

type Account struct {
...
SequenceNumber int64
+ Gaps *IntSet
}
yihuang marked this conversation as resolved.
Show resolved Hide resolved

// CheckNonce checks if the input nonce is valid, if yes, modify internal state.
func(acc *Account) CheckNonce(nonce int64) error {
switch {
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
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)
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.

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

```

Prototype implementation of `IntSet`:

```golang
type IntSet struct {
capacity int
bitmap roaringbitmap.BitMap
}

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(),
}

}

func (is *IntSet) Add(n int) {
if is.bitmap.Length() >= is.capacity {
// 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.

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.

}

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.


func (is *IntSet) Remove(n int) {
is.bitmap.Remove(n)
}

func (is *IntSet) Contains(n int) bool {
return is.bitmap.Contains(n)
}
```

## 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


## Consequences
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved

### Positive

* Support concurrent transaction inclusion.
* Only optional fields are added to `Account`, migration is easy.

### Negative

- Limited runtime overhead.

## References

* https://github.com/cosmos/cosmos-sdk/issues/13009
Loading