From 59f9c1b6bfa5b38e4cbf8d7db5d16a84470fea9e Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Tue, 27 Jul 2021 10:35:09 +0200 Subject: [PATCH] feat: TxRaw must follow ADR 027 (#9743) (#9754) ## Description Closes: #XXXX --- ### 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... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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) (cherry picked from commit adff7d804d3fbbf332c8a5967889ff9837c92f4e) Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com> --- x/auth/tx/decoder.go | 41 ++++++++++++++++++----------------------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/x/auth/tx/decoder.go b/x/auth/tx/decoder.go index 2fef6312b994..d93c7446db60 100644 --- a/x/auth/tx/decoder.go +++ b/x/auth/tx/decoder.go @@ -16,7 +16,7 @@ import ( func DefaultTxDecoder(cdc codec.ProtoCodecMarshaler) sdk.TxDecoder { return func(txBytes []byte) (sdk.Tx, error) { // Make sure txBytes follow ADR-027. - err := rejectNonADR027TxRaw(txBytes) + err := rejectNonADR027(txBytes) if err != nil { return nil, sdkerrors.Wrap(sdkerrors.ErrTxDecode, err.Error()) } @@ -90,14 +90,13 @@ func DefaultJSONTxDecoder(cdc codec.ProtoCodecMarshaler) sdk.TxDecoder { } } -// rejectNonADR027TxRaw rejects txBytes that do not follow ADR-027. This is NOT -// a generic ADR-027 checker, it only applies decoding TxRaw. Specifically, it +// rejectNonADR027 rejects txBytes that do not follow ADR-027. This function // only checks that: // - field numbers are in ascending order (1, 2, and potentially multiple 3s), -// - and varints are as short as possible. -// All other ADR-027 edge cases (e.g. default values) are not applicable with -// TxRaw. -func rejectNonADR027TxRaw(txBytes []byte) error { +// - and varints as as short as possible. +// All other ADR-027 edge cases (e.g. TxRaw fields having default values) will +// not happen with TxRaw. +func rejectNonADR027(txBytes []byte) error { // Make sure all fields are ordered in ascending order with this variable. prevTagNum := protowire.Number(0) @@ -106,25 +105,21 @@ func rejectNonADR027TxRaw(txBytes []byte) error { if m < 0 { return fmt.Errorf("invalid length; %w", protowire.ParseError(m)) } - // TxRaw only has bytes fields. if wireType != protowire.BytesType { - return fmt.Errorf("expected %d wire type, got %d", protowire.BytesType, wireType) + return fmt.Errorf("expected %d wire type, got %d", protowire.VarintType, wireType) } - // Make sure fields are ordered in ascending order. if tagNum < prevTagNum { return fmt.Errorf("txRaw must follow ADR-027, got tagNum %d after tagNum %d", tagNum, prevTagNum) } prevTagNum = tagNum // All 3 fields of TxRaw have wireType == 2, so their next component - // is a varint, so we can safely call ConsumeVarint here. - // Byte structure: - // Inner fields are verified in `DefaultTxDecoder` + // is a varint. + // We make sure that the varint is as short as possible. lengthPrefix, m := protowire.ConsumeVarint(txBytes[m:]) if m < 0 { return fmt.Errorf("invalid length; %w", protowire.ParseError(m)) } - // We make sure that this varint is as short as possible. n := varintMinLength(lengthPrefix) if n != m { return fmt.Errorf("length prefix varint for tagNum %d is not as short as possible, read %d, only need %d", tagNum, m, n) @@ -146,23 +141,23 @@ func rejectNonADR027TxRaw(txBytes []byte) error { func varintMinLength(n uint64) int { switch { // Note: 1<