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

fix!: use correct chain-id when filtering and checking txs #2022

Merged
merged 5 commits into from
Jul 4, 2023

Conversation

evan-forbes
Copy link
Member

Overview

consistently pass the correct chain-id to the application using RequestPrepareProposal and RequestProcessProposal

this PR still needs to update the go.mod to use the official tag of tendermint after its released

closes #2020 and #2021

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@evan-forbes evan-forbes added the bug Something isn't working label Jul 4, 2023
@evan-forbes evan-forbes added this to the Mainnet milestone Jul 4, 2023
@evan-forbes evan-forbes self-assigned this Jul 4, 2023
@MSevey MSevey requested a review from a team July 4, 2023 12:46
Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

unit tests failing?

@MSevey MSevey requested a review from a team July 4, 2023 13:14
@evan-forbes
Copy link
Member Author

evan-forbes commented Jul 4, 2023

unit tests failing?

yeah sorry, waiting for tests to finish locally but they should be fixed

edit: gahhh one sec these tests need to be updated... we can review celestiaorg/celestia-core#1028 tho

rach-id
rach-id previously approved these changes Jul 4, 2023
Copy link
Member

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

:shipit:

test/util/testfactory/blob.go Show resolved Hide resolved
Comment on lines +82 to +85
func SortBlobs(blobs []types.Blob) []types.Blob {
sort.Slice(blobs, func(i, j int) bool { return bytes.Compare(blobs[i].NamespaceID, blobs[j].NamespaceID) < 0 })
return blobs
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

[not blocking] I think there's a subtle bug here b/c this comparison doesn't account for the namespace version. I can fix in a follow up PR or this can use something like:

// sort the blobs in order of namespace. We use slice stable here to respect the
// order of multiple blobs within a namespace as per the priority of the PFB
sort.SliceStable(b.blobs, func(i, j int) bool {
return bytes.Compare(fullNamespace(b.blobs[i].blob), fullNamespace(b.blobs[j].blob)) < 0
})

Suggested change
func SortBlobs(blobs []types.Blob) []types.Blob {
sort.Slice(blobs, func(i, j int) bool { return bytes.Compare(blobs[i].NamespaceID, blobs[j].NamespaceID) < 0 })
return blobs
}
func SortBlobs(blobs []types.Blob) []types.Blob {
sort.SliceStable(blobs, func(i, j int) bool {
return bytes.Compare(fullNamespace(blobs[i]), fullNamespace(blobs[j])) < 0
})
return blobs
}
// TODO: celestia-core should provide this method for `Blob`s
func fullNamespace(blob types.Blob) []byte {
return append([]byte{byte(blob.NamespaceVersion)}, blob.NamespaceID...)
}

not blocking b/c in practice I can't think of a scenario where not accounting for the namespace version would result in a different ordering b/c there are only two versions (0 and 255) and the reserved namespaces that use 255 have IDs that would be ordered after every blob namespace ID

Copy link
Member Author

Choose a reason for hiding this comment

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

good find, and yeah this is using the same as what we were using before (only for tests as well I believe)

@@ -201,5 +201,5 @@ replace (
github.com/cosmos/cosmos-sdk => github.com/celestiaorg/cosmos-sdk v1.15.0-sdk-v0.46.13
github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1
github.com/syndtr/goleveldb => github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7
github.com/tendermint/tendermint => github.com/celestiaorg/celestia-core v1.22.0-tm-v0.34.28
github.com/tendermint/tendermint => github.com/celestiaorg/celestia-core v1.23.0-tm-v0.34.28
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@codecov-commenter
Copy link

Codecov Report

Merging #2022 (9091c3c) into main (30c50a0) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main    #2022   +/-   ##
=======================================
  Coverage   21.54%   21.54%           
=======================================
  Files         127      127           
  Lines       14293    14293           
=======================================
  Hits         3079     3079           
  Misses      10922    10922           
  Partials      292      292           
Impacted Files Coverage Δ
app/prepare_proposal.go 0.00% <0.00%> (ø)
app/process_proposal.go 0.00% <0.00%> (ø)

@evan-forbes evan-forbes changed the title fix: use correct chain-id when filtering and checking txs fix!: use correct chain-id when filtering and checking txs Jul 4, 2023
@evan-forbes evan-forbes merged commit 965eafb into main Jul 4, 2023
@evan-forbes evan-forbes deleted the evan/pass-chain-id-to-prepare branch July 4, 2023 14:05
evan-forbes added a commit that referenced this pull request Jul 4, 2023
consistently pass the correct chain-id to the application using
`RequestPrepareProposal` and `RequestProcessProposal`

this PR still needs to update the go.mod to use the official tag of
tendermint after its released

closes #2020 and #2021

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords
@liamsi liamsi mentioned this pull request Jul 4, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bank/tx: Can't send funds to account address from validator
5 participants