-
Notifications
You must be signed in to change notification settings - Fork 4k
docs: adr-070 unordered transactions refactor #23974
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
Changes from 1 commit
8e5fad5
6a94007
ff071b0
42f5a85
f789622
00c9dc7
2b2fd56
7567f7d
12c9ff5
0ad74a1
8ea4c05
50c1f93
8f7da10
932644f
eb63931
580cd2b
8cffb12
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,8 +32,8 @@ below, without impacting traditional ordered transactions which will follow the | |
We will introduce new storage of time-based, ephemeral unordered sequences using the SDK's existing KV Store library. | ||
Specifically, we will leverage the existing x/auth KV store to store the unordered sequences. | ||
|
||
When an unordered transaction is included in a block, a concatenation of the `timeout_timestamp` and sender’s PubKey | ||
bytes will be recorded to state (i.e. `542939323/<pubkey_btyes>`). In cases of multi-party signing, one entry per signer | ||
When an unordered transaction is included in a block, a concatenation of the `timeout_timestamp` and sender’s address bytes | ||
will be recorded to state (i.e. `542939323/<address_bytes>`). In cases of multi-party signing, one entry per signer | ||
will be recorded to state. | ||
|
||
New transactions will be checked against the state to prevent duplicate submissions. To prevent the state from growing indefinitely, we propose the following: | ||
|
@@ -63,7 +63,7 @@ will be deleted. | |
|
||
```go | ||
func (am AppModule) PreBlock(ctx context.Context) (appmodule.ResponsePreBlock, error) { | ||
err := am.accountKeeper.GetUnorderedTxManager().RemoveExpired(sdk.UnwrapSDKContext(ctx)) | ||
err := am.accountKeeper.RemoveExpired(sdk.UnwrapSDKContext(ctx)) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
@@ -86,32 +86,20 @@ var ( | |
unorderedSequencePrefix = collections.NewPrefix(90) | ||
) | ||
|
||
type UnorderedTxManager struct { | ||
type AccountKeeper struct { | ||
// ... | ||
unorderedSequences collections.KeySet[collections.Pair[uint64, []byte]] | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What module do we want to have this in? All KV store keys need a module. x/auth seems the most logical, but we could also have a small dedicated x/auth/unordered module. Also, I think we can just use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
are there tradeoffs between doing one or the other?
i was toying around with this, but was having trouble figuring out how to iterate from nil to a partial value of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. x/auth means one less module to wire up in app.go, but maybe more tie in to x/auth long term. This is maybe a longer discussion. But for easy upgrades in 0.53, I would say x/auth and we expose a simple, well defined interface for it. Collections isn't essential here I think, just thought it might be easier. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i have a PoC where i've made unordered sequences managed directly by x/auth store/keeper. you can check it out here: #24010 i also switched to using collections after playing with the API more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks generally good, but as a practice for ADR I don't find it particularly useful to have this level of detail around code implementation (I realize the previous iteration had a lot of detail too). The more important thing IMHO is the conceptual flow, i.e. pseudo-code - what happens in each phase, what properties of security, performance, etc. does the code have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes sense - i did find it cumbersome going back and forth between this and the draft. throwing some simple python in here would be easier to deal with |
||
|
||
func NewUnorderedTxManager(kvStore store.KVStoreService) *UnorderedTxManager { | ||
sb := collections.NewSchemaBuilder(kvStore) | ||
m := &UnorderedTxManager{ | ||
unorderedSequences: collections.NewKeySet( | ||
sb, | ||
unorderedSequencePrefix, | ||
"unordered_sequences", | ||
collections.PairKeyCodec(collections.Uint64Key, collections.BytesKey), | ||
), | ||
} | ||
return m | ||
} | ||
|
||
func (m *UnorderedTxManager) Contains(ctx sdk.Context, sender []byte, timestamp uint64) (bool, error) { | ||
func (m *AccountKeeper) Contains(ctx sdk.Context, sender []byte, timestamp uint64) (bool, error) { | ||
return m.unorderedSequences.Has(ctx, collections.Join(timestamp, sender)) | ||
} | ||
|
||
func (m *UnorderedTxManager) Add(ctx sdk.Context, sender []byte, timestamp uint64) error { | ||
func (m *AccountKeeper) Add(ctx sdk.Context, sender []byte, timestamp uint64) error { | ||
return m.unorderedSequences.Set(ctx, collections.Join(timestamp, sender)) | ||
} | ||
|
||
func (m *UnorderedTxManager) RemoveExpired(ctx sdk.Context) error { | ||
func (m *AccountKeeper) RemoveExpired(ctx sdk.Context) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would just suggest something a little more detailed than simply |
||
blkTime := ctx.BlockTime().UnixNano() | ||
it, err := m.unorderedSequences.Iterate(ctx, collections.NewPrefixUntilPairRange[uint64, []byte](uint64(blkTime))) | ||
if err != nil { | ||
|
@@ -182,15 +170,15 @@ var _ sdk.AnteDecorator = (*UnorderedTxDecorator)(nil) | |
// a duplicate and will evict it from state when the timeout is reached. | ||
// | ||
// The UnorderedTxDecorator should be placed as early as possible in the AnteHandler | ||
// chain to ensure that during DeliverTx, the transaction is added to the UnorderedTxManager. | ||
// chain to ensure that during DeliverTx, the transaction is added to the unordered sequence state. | ||
type UnorderedTxDecorator struct { | ||
// maxUnOrderedTTL defines the maximum TTL a transaction can define. | ||
maxTimeoutDuration time.Duration | ||
txManager *authkeeper.UnorderedTxManager | ||
txManager authkeeper.UnorderedTxManager | ||
} | ||
|
||
func NewUnorderedTxDecorator( | ||
utxm *authkeeper.UnorderedTxManager, | ||
utxm authkeeper.UnorderedTxManager, | ||
) *UnorderedTxDecorator { | ||
return &UnorderedTxDecorator{ | ||
maxTimeoutDuration: 10 * time.Minute, | ||
|
@@ -282,17 +270,7 @@ func getSigners(tx sdk.Tx) ([][]byte, error) { | |
if !ok { | ||
return nil, errorsmod.Wrap(sdkerrors.ErrTxDecode, "invalid tx type") | ||
} | ||
sigs, err := sigTx.GetSignaturesV2() | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
addresses := make([][]byte, 0, len(sigs)) | ||
for _, sig := range sigs { | ||
addresses = append(addresses, sig.PubKey.Address().Bytes()) | ||
} | ||
|
||
return addresses, nil | ||
return sigTx.GetSigners() | ||
} | ||
|
||
``` | ||
|
Uh oh!
There was an error while loading. Please reload this page.