docs: adr-070 unordered transactions refactor #23974

technicallyty · 2025-03-12T20:29:54Z

Description

new way of doing unordered txs

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, you can find examples of the prefixes below:
confirmed ! in 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
reviewed "Files changed" and left comments if necessary
included the necessary unit and integration tests
added a changelog entry to CHANGELOG.md
updated the relevant documentation or specification, including comments for documenting Go code
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.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

I have...

confirmed the correct type prefix in the PR title
confirmed all author checklist items have been addressed
reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

docs/architecture/adr-070-unordered-account.md

aaronc · 2025-03-13T21:08:39Z

docs/architecture/adr-070-unordered-account.md

+// Manager contains the tx hash dictionary for duplicates checking, and expire
+// them when block production progresses.
+type Manager struct {
+  kvStore store.KVStoreService
 }


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 collections.Set to deal with state here.

What module do we want to have this in?

are there tradeoffs between doing one or the other?

Also, I think we can just use collections.Set to deal with state here.

i was toying around with this, but was having trouble figuring out how to iterate from nil to a partial value of <block_time/.

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.

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.

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

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

Co-authored-by: Alex | Interchain Labs <[email protected]>

…smos/cosmos-sdk into technicallyty/new-unordered-adr

aljo242 · 2025-03-17T22:11:09Z

docs/architecture/adr-070-unordered-account.md

-* Jan 30, 2024: Include section on deterministic transaction encoding
+- Dec 4, 2023: Initial Draft (@yihuang, @tac0turtle, @alexanderbez)
+- Jan 30, 2024: Include section on deterministic transaction encoding
+- Mar 12, 2025: Revise implementation to use Cosmos SDK KV Store and require unique timeouts per-address (@technicallyty)


Update date whenever we merge

aljo242 · 2025-03-17T22:11:18Z

docs/architecture/adr-070-unordered-account.md


 ## Status

-ACCEPTED
+Draft


Update to accpeted whenever we merge

So if we're following PROCESS.md, this should probably be ACCEPTED Not Implemented

aljo242 · 2025-03-17T22:12:34Z

docs/architecture/adr-070-unordered-account.md

-and limit the size of the dictionary.
+When an unordered transaction is included in a block, a concatenation of the `timeout_timestamp` and sender’s bech32 address
+will be recorded to state (i.e. `542939323/cosmos1v1234567890AbcDeF`). In cases of multi-party signing, we will use a
+comma-separated list of the addresses that signed the transaction (i.e. `5532231/cosmosv11,cosmosv12,cosmosv13`)


we should specify that this is sorted (and implement it as sorted)

theoretically you could submit two Tx with the same timestamp with with A, B, C signers and B, A, C signers right?

These are effectively the same Tx but in the store (if not sorted) would be considered two separate

aljo242 · 2025-03-17T22:14:55Z

docs/architecture/adr-070-unordered-account.md

-  return nil
+```go
+func (am AppModule) PreBlock(ctx context.Context) (appmodule.ResponsePreBlock, error) {
+	err := am.accountKeeper.GetUnorderedTxManager().RemoveExpired(sdk.UnwrapSDKContext(ctx))


we should have NPE protection on this for when users do not use this

the account keeper currently always has this instantiated, so it wouldn't NPE

you can specify to enable the unordered ante in the ante options, but we'd need another option if we want to enable/disable constructing this field in the account keeper, replacing it with a no-op impl maybe.. might be awkward UX to specify this in two different places though. probably best to walk through this on a call

Yeah I see your point - lets just be aware of it when we do code reviews for this. I think you're right about how it is awkward if users have to set in multiple places

I think it's just simpler to add these methods directly to the account keeper expected interface. There's not much advantage to needed to get an unordered tx manager from the account keeper IMHO. Simpler just to have something like:

type AccountKeeper interface { ... TryAddUnorderedNonce(context,Context, timestamp Timestamp, address []byte) error CleanupUnorderedNonces(context.Context) }

Any thoughts here? Looks like this was marked as resolved

docs/architecture/adr-070-unordered-account.md

aaronc

Left a few comments - maybe some small decisions to think through, but generally looks good.

docs/architecture/adr-070-unordered-account.md

aaronc · 2025-03-18T16:13:44Z

docs/architecture/adr-070-unordered-account.md

-safety, we should limit the range of `timeout_timestamp` to prevent very long expiration,
-and limit the size of the dictionary.
+When an unordered transaction is included in a block, a concatenation of the `timeout_timestamp` and sender’s bech32 address
+will be recorded to state (i.e. `542939323/cosmos1v1234567890AbcDeF`). In cases of multi-party signing, we will use a sorted,


Why bech32 encoded? Shouldn't we just use the binary address bytes?

i'll change it to bytes. the PoC used the pubkey.address.string() but i can just use bytes

actually, this complicates the impl a bit, since i was using the concat of pubkey address strings. lmk what you think

since the update to one entry per signer, i have changed it to just use the pubkey bytes.

So we want to use the address, not the pubkey bytes which could depend on how the pubkey is serialized. As an alternative to the address that may be even better is just using the account number which is just a uint64

aaronc · 2025-03-18T16:16:56Z

docs/architecture/adr-070-unordered-account.md

-  return nil
+```go
+func (am AppModule) PreBlock(ctx context.Context) (appmodule.ResponsePreBlock, error) {
+	err := am.accountKeeper.GetUnorderedTxManager().RemoveExpired(sdk.UnwrapSDKContext(ctx))


I think it's just simpler to add these methods directly to the account keeper expected interface. There's not much advantage to needed to get an unordered tx manager from the account keeper IMHO. Simpler just to have something like:

type AccountKeeper interface { ... TryAddUnorderedNonce(context,Context, timestamp Timestamp, address []byte) error CleanupUnorderedNonces(context.Context) }

aaronc · 2025-03-18T16:18:39Z

docs/architecture/adr-070-unordered-account.md

+// Manager contains the tx hash dictionary for duplicates checking, and expire
+// them when block production progresses.
+type Manager struct {
+  kvStore store.KVStoreService
 }


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

docs/architecture/adr-070-unordered-account.md

github-actions · 2025-03-18T17:02:19Z

⚠️ govulncheck found vulnerabilities:

mkdir -p /home/runner/work/cosmos-sdk/cosmos-sdk/build/
GOBIN=/home/runner/work/cosmos-sdk/cosmos-sdk/build go install golang.org/x/vuln/cmd/govulncheck@latest
go: downloading golang.org/x/vuln v1.1.4
go: downloading golang.org/x/telemetry v0.0.0-20240522233618-39ace7a40ae7
go: downloading golang.org/x/mod v0.22.0
go: downloading golang.org/x/tools v0.29.0
/home/runner/work/cosmos-sdk/cosmos-sdk/build/govulncheck ./...
=== Symbol Results ===

Vulnerability #1: GO-2025-3443
    CometBFT allows a malicious peer to stall the network by disseminating
    seemingly valid block parts in github.com/cometbft/cometbft
  More info: https://pkg.go.dev/vuln/GO-2025-3443
  Module: github.com/cometbft/cometbft
    Found in: github.com/cometbft/[email protected]
    Fixed in: github.com/cometbft/[email protected]
    Example traces found:
      #1: testutil/network/util.go:74:24: network.startInProcess calls service.BaseService.Start, which eventually calls types.Part.ValidateBasic
      #2: testutil/network/util.go:74:24: network.startInProcess calls service.BaseService.Start, which eventually calls types.Part.ValidateBasic
      #3: client/rpc/block.go:56:36: rpc.QueryBlocks calls local.Local.BlockSearch, which eventually calls types.PartFromProto
      #4: client/rpc/block.go:56:36: rpc.QueryBlocks calls local.Local.BlockSearch, which eventually calls types.PartFromProto

Your code is affected by 1 vulnerability from 1 module.
This scan also found 0 vulnerabilities in packages you import and 2
vulnerabilities in modules you require, but your code doesn't appear to call
these vulnerabilities.
Use '-show verbose' for more details.
make: *** [Makefile:155: vulncheck] Error 3

…ad of separate manager struct

github-actions · 2025-03-19T16:59:59Z

    ⚠️ **govulncheck found vulnerabilities:**


    >mkdir -p /home/runner/work/cosmos-sdk/cosmos-sdk/build/

GOBIN=/home/runner/work/cosmos-sdk/cosmos-sdk/build go install golang.org/x/vuln/cmd/govulncheck@latest
go: downloading golang.org/x/vuln v1.1.4
go: downloading golang.org/x/telemetry v0.0.0-20240522233618-39ace7a40ae7
go: downloading golang.org/x/mod v0.22.0
go: downloading golang.org/x/tools v0.29.0
/home/runner/work/cosmos-sdk/cosmos-sdk/build/govulncheck ./...
=== Symbol Results ===

Vulnerability #1: GO-2025-3443
CometBFT allows a malicious peer to stall the network by disseminating
seemingly valid block parts in github.com/cometbft/cometbft
More info: https://pkg.go.dev/vuln/GO-2025-3443
Module: github.com/cometbft/cometbft
Found in: github.com/cometbft/[email protected]
Fixed in: github.com/cometbft/[email protected]
Example traces found:
#1: testutil/network/util.go:74:24: network.startInProcess calls service.BaseService.Start, which eventually calls types.Part.ValidateBasic
#2: testutil/network/util.go:74:24: network.startInProcess calls service.BaseService.Start, which eventually calls types.Part.ValidateBasic
#3: client/rpc/block.go:56:36: rpc.QueryBlocks calls local.Local.BlockSearch, which eventually calls types.PartFromProto
#4: client/rpc/block.go:56:36: rpc.QueryBlocks calls local.Local.BlockSearch, which eventually calls types.PartFromProto

Your code is affected by 1 vulnerability from 1 module.
This scan also found 0 vulnerabilities in packages you import and 2
vulnerabilities in modules you require, but your code doesn't appear to call
these vulnerabilities.
Use '-show verbose' for more details.
make: *** [Makefile:155: vulncheck] Error 3

aaronc

I think we've addressed the key issues 👍. We can deal with details in the implementation PR

aaronc · 2025-03-19T17:29:01Z

docs/architecture/adr-070-unordered-account.md

+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 {


I would just suggest something a little more detailed than simply Contains, Add, etc. that mentions unordered

github-actions · 2025-03-19T19:44:51Z

    ⚠️ **govulncheck found vulnerabilities:**


    >mkdir -p /home/runner/work/cosmos-sdk/cosmos-sdk/build/

GOBIN=/home/runner/work/cosmos-sdk/cosmos-sdk/build go install golang.org/x/vuln/cmd/govulncheck@latest
go: downloading golang.org/x/vuln v1.1.4
go: downloading golang.org/x/telemetry v0.0.0-20240522233618-39ace7a40ae7
go: downloading golang.org/x/mod v0.22.0
go: downloading golang.org/x/tools v0.29.0
/home/runner/work/cosmos-sdk/cosmos-sdk/build/govulncheck ./...
=== Symbol Results ===

Vulnerability #1: GO-2025-3443
CometBFT allows a malicious peer to stall the network by disseminating
seemingly valid block parts in github.com/cometbft/cometbft
More info: https://pkg.go.dev/vuln/GO-2025-3443
Module: github.com/cometbft/cometbft
Found in: github.com/cometbft/[email protected]
Fixed in: github.com/cometbft/[email protected]
Example traces found:
#1: testutil/network/util.go:74:24: network.startInProcess calls service.BaseService.Start, which eventually calls types.Part.ValidateBasic
#2: testutil/network/util.go:74:24: network.startInProcess calls service.BaseService.Start, which eventually calls types.Part.ValidateBasic
#3: client/rpc/block.go:56:36: rpc.QueryBlocks calls local.Local.BlockSearch, which eventually calls types.PartFromProto
#4: client/rpc/block.go:56:36: rpc.QueryBlocks calls local.Local.BlockSearch, which eventually calls types.PartFromProto

Your code is affected by 1 vulnerability from 1 module.
This scan also found 0 vulnerabilities in packages you import and 2
vulnerabilities in modules you require, but your code doesn't appear to call
these vulnerabilities.
Use '-show verbose' for more details.
make: *** [Makefile:155: vulncheck] Error 3

new adr

8e5fad5

github-actions bot added the Type: ADR label Mar 12, 2025

technicallyty added 2 commits March 12, 2025 15:09

clean

6a94007

add note about multi signer

ff071b0

technicallyty mentioned this pull request Mar 13, 2025

Implement unordered tx PoC using state store #23976

Open

technicallyty marked this pull request as ready for review March 13, 2025 16:38

technicallyty requested a review from a team as a code owner March 13, 2025 16:38

aljo242 reviewed Mar 13, 2025

View reviewed changes

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

aljo242 reviewed Mar 13, 2025

View reviewed changes

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

aljo242 reviewed Mar 13, 2025

View reviewed changes

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

aljo242 reviewed Mar 13, 2025

View reviewed changes

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

aljo242 reviewed Mar 13, 2025

View reviewed changes

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

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

add note about previous iteration

42f5a85

aaronc reviewed Mar 13, 2025

View reviewed changes

technicallyty and others added 6 commits March 13, 2025 15:32

Update docs/architecture/adr-070-unordered-account.md

f789622

Co-authored-by: Alex | Interchain Labs <[email protected]>

additions'

00c9dc7

Merge branch 'technicallyty/new-unordered-adr' of ssh://github.com/co…

2b2fd56

…smos/cosmos-sdk into technicallyty/new-unordered-adr

update code

7567f7d

note on malleability

12c9ff5

add note on x/auth kv store

0ad74a1

aljo242 reviewed Mar 17, 2025

View reviewed changes

sorted

8ea4c05

aljo242 approved these changes Mar 18, 2025

View reviewed changes

aaronc reviewed Mar 18, 2025

View reviewed changes

technicallyty added 2 commits March 18, 2025 09:54

not uint64, time

50c1f93

updates

8f7da10

technicallyty added 2 commits March 18, 2025 13:01

edits about single entry per signer

932644f

some edits

eb63931

aljo242 approved these changes Mar 19, 2025

View reviewed changes

use GetSigners and lets place methds directly on account keeper inste…

580cd2b

…ad of separate manager struct

aaronc approved these changes Mar 19, 2025

View reviewed changes

Merge branch 'release/v0.53.x' into technicallyty/new-unordered-adr

8cffb12

technicallyty enabled auto-merge (squash) March 19, 2025 19:42

technicallyty merged commit 6577e1e into release/v0.53.x Mar 19, 2025
43 of 44 checks passed

technicallyty deleted the technicallyty/new-unordered-adr branch March 19, 2025 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: adr-070 unordered transactions refactor #23974

docs: adr-070 unordered transactions refactor #23974

technicallyty commented Mar 12, 2025

aaronc Mar 13, 2025

technicallyty Mar 13, 2025

aaronc Mar 17, 2025

technicallyty Mar 17, 2025 •

edited

Loading

aaronc Mar 18, 2025

technicallyty Mar 18, 2025

aljo242 Mar 17, 2025

aljo242 Mar 17, 2025

aaronc Mar 18, 2025

aljo242 Mar 17, 2025

aljo242 Mar 17, 2025

aljo242 Mar 17, 2025

technicallyty Mar 17, 2025 •

edited

Loading

aljo242 Mar 18, 2025

aaronc Mar 18, 2025

aaronc Mar 19, 2025

aaronc left a comment

aaronc Mar 18, 2025

technicallyty Mar 18, 2025

technicallyty Mar 18, 2025

technicallyty Mar 18, 2025

aaronc Mar 19, 2025

aaronc Mar 18, 2025

aaronc Mar 18, 2025

github-actions bot commented Mar 18, 2025 •

edited

Loading

github-actions bot commented Mar 19, 2025

aaronc left a comment

aaronc Mar 19, 2025

github-actions bot commented Mar 19, 2025

docs: adr-070 unordered transactions refactor #23974

docs: adr-070 unordered transactions refactor #23974

Conversation

technicallyty commented Mar 12, 2025

Description

Author Checklist

Reviewers Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

technicallyty Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

technicallyty Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 18, 2025 • edited Loading

github-actions bot commented Mar 19, 2025

aaronc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 19, 2025

technicallyty Mar 17, 2025 •

edited

Loading

technicallyty Mar 17, 2025 •

edited

Loading

github-actions bot commented Mar 18, 2025 •

edited

Loading