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

chore: bump upstream v0.46.13 #324

Merged
merged 86 commits into from
Jun 13, 2023

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented Jun 9, 2023

Description

pulls the latest from upstream, which include bareberry patch. also bumps core to v0.34.28

closes #310

ready for review, but blocked by celestiaorg/celestia-core#1021

julienrbrt and others added 30 commits November 22, 2022 15:54
…s#13996)

* chore: add group and gov/v1 in swagger (cosmos#13984)

Co-authored-by: Marko <marbar3778@yahoo.com>
(cherry picked from commit cfa224c)

# Conflicts:
#	client/docs/config.json
#	client/docs/swagger-ui/swagger.yaml

* updates

Co-authored-by: Julien Robert <julien@rbrt.fr>
…4071) (cosmos#14091)

* fix(group)!: Don't re-tally proposals after VP end (cosmos#14071)

(cherry picked from commit 4ebe3aa)

# Conflicts:
#	CHANGELOG.md

* fix changelog and test

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
…mos#14101)

* fix: do not shadow clientCtx in start.go (cosmos#14086)

(cherry picked from commit f96072d)

# Conflicts:
#	server/start.go

* updates

Co-authored-by: Julien Robert <julien@rbrt.fr>
(cherry picked from commit 9f46665)

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
* fix: remove duplicate ante events (cosmos#13983)

## Description

After enabling the post-handler on our chain, we realised something odd with our transaction events. The ordering was `[ANTE, MSGS, ANTE, POST]`. After doing some digging, we realised that the context used inside the post-handler still contains the events from the ante-handler.

This should be backported to v0.46.x and v0.47.x 😄
Happy to make it compatible with cosmos#13940 when that gets merged!

cc @mbreithecker

---

### 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](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/main/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/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/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 25449b5)

# Conflicts:
#	CHANGELOG.md

* fix changelog

Co-authored-by: John Letey <john@kyve.network>
Co-authored-by: Julien Robert <julien@rbrt.fr>
…osmos#14169)

* feat: support alternative query multistore (cosmos#13529)

* support customize query multistore

* Update CHANGELOG.md

* fix test

* Update baseapp/abci.go

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

* Update baseapp/baseapp.go

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

* Update baseapp/options.go

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: Marko <marbar3778@yahoo.com>
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
(cherry picked from commit 15accd7)

# Conflicts:
#	CHANGELOG.md
#	baseapp/baseapp.go
#	baseapp/options.go

* fix conflicts

Co-authored-by: yihuang <huang@crypto.com>
…) (cosmos#14138)

* fix: state listener observe writes at wrong time (cosmos#13516)

* fix: state listener observe writes at wrong time

Closes: cosmos#13457

Currently state listener is notified when the cache store write, which happens in commit event only, which breaks the current design.
The solution (as discussed in the issue) is to listen state writes on rootmulti store only.

It also changes the file streamer to output single data file for the writes in the whole block, since we can't distinguish writes from different stage of abci events.

It adds new config items for file streamer:
- streamers.file.output-metadata
- streamers.file.stop-node-on-error
- streamers.file.fsync

* synchronous abci call, and format doc

* fix comment

* update file streamer readme and fix typos

* typo

* fix: state listener observe writes at wrong time

Closes: cosmos#13457

Currently state listener is notified when the cache store write, which happens in commit event only, which breaks the current design.
The solution (as discussed in the issue) is to listen state writes on rootmulti store only.

It also changes the file streamer to output single data file for the writes in the whole block, since we can't distinguish writes from different stage of abci events.

It adds new config items for file streamer:
- streamers.file.output-metadata
- streamers.file.stop-node-on-error
- streamers.file.fsync

synchronous abci call, and format doc

fix comment

update file streamer readme and fix typos

typo

* improve UX of file streamer, make it immediately usable after enabled

- set default value to write_dir.
- make write_dir based on home directory by default.
- auto-create the directory if not exists.

* get homePage from opts

Co-authored-by: Marko <marbar3778@yahoo.com>
(cherry picked from commit 1f91ee2)

# Conflicts:
#	CHANGELOG.md
#	api/cosmos/base/store/v1beta1/listening.pulsar.go
#	baseapp/streaming.go
#	docs/architecture/adr-038-state-listening.md
#	server/config/toml.go
#	simapp/app_v2.go
#	store/cachemulti/store.go
#	store/iavl/store.go
#	store/mem/store.go
#	store/streaming/constructor.go
#	store/streaming/file/service.go
#	store/streaming/file/service_test.go
#	store/types/listening.pb.go
#	store/types/store.go

* `make proto-gen`, update changelog, delete uncessary files

* fix conflicts

* fix conflicts

* revert api breaking change

* fix build

* fix unit test

Co-authored-by: yihuang <huang@crypto.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
Co-authored-by: Marko <marbar3778@yahoo.com>
cosmos#14178)

* refactor: provide a helper for baseapp options (cosmos#14175)

* provide a helper for baseapp options

* rename

* changelog entry

* fix spelling

(cherry picked from commit 1b6192f)

# Conflicts:
#	CHANGELOG.md
#	server/mock/app_test.go
#	simapp/simd/cmd/root.go

* conflicts

* lint

Co-authored-by: Marko <marbar3778@yahoo.com>
Co-authored-by: Aaron Craelius <aaron@regen.network>
Co-authored-by: Julien Robert <julien@rbrt.fr>
…osmos#14241)

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
…mos#14305) (cosmos#14309)

Co-authored-by: Daniel Wedul <github@wedul.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
Co-authored-by: yihuang <huang@crypto.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
Co-authored-by: Matt Kocubinski <mkocubinski@gmail.com>
…) (cosmos#14638)

Co-authored-by: Julien Robert <julien@rbrt.fr>
Co-authored-by: Julien Robert <julien@rbrt.fr>
…mos#14799)

Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com>
Co-authored-by: HuangYi <huang@crypto.com>
Co-authored-by: Julien Robert <julien@rbrt.fr>
Co-authored-by: Marko <marbar3778@yahoo.com>
…tore (backport cosmos#14189) (cosmos#14862)

Co-authored-by: yihuang <huang@crypto.com>
Co-authored-by: marbar3778 <marbar3778@yahoo.com>
@evan-forbes evan-forbes added the dependencies Pull requests that update a dependency file label Jun 9, 2023
@evan-forbes evan-forbes added this to the Mainnet milestone Jun 9, 2023
@evan-forbes evan-forbes requested a review from liamsi as a code owner June 9, 2023 02:00
@evan-forbes evan-forbes self-assigned this Jun 9, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2023

Codecov Report

Merging #324 (643c2b8) into release/v0.46.x-celestia (390a17f) will decrease coverage by 0.03%.
The diff coverage is 50.65%.

Impacted file tree graph

@@                     Coverage Diff                      @@
##           release/v0.46.x-celestia     #324      +/-   ##
============================================================
- Coverage                     65.54%   65.52%   -0.03%     
============================================================
  Files                           666      666              
  Lines                         71016    71263     +247     
============================================================
+ Hits                          46550    46693     +143     
- Misses                        21884    21976      +92     
- Partials                       2582     2594      +12     
Impacted Files Coverage Δ
baseapp/msg_service_router.go 62.50% <0.00%> (-9.87%) ⬇️
simapp/state.go 0.00% <0.00%> (ø)
types/simulation/types.go 0.00% <ø> (ø)
x/auth/vesting/types/msgs.go 0.00% <0.00%> (ø)
x/group/client/cli/query.go 0.00% <0.00%> (ø)
x/group/keeper/grpc_query.go 60.76% <0.00%> (-4.53%) ⬇️
baseapp/baseapp.go 77.63% <27.27%> (-0.99%) ⬇️
snapshots/manager.go 45.42% <42.37%> (-2.16%) ⬇️
x/group/keeper/keeper.go 56.34% <60.00%> (+1.00%) ⬆️
x/group/keeper/msg_server.go 68.63% <62.50%> (-0.07%) ⬇️
... and 10 more

... and 1 file with indirect coverage changes

"fmt"
"io"
"os"
"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import

Certain system packages contain functions which may be a possible source of non-determinism
Comment on lines +68 to +77
go func() {
defer close(quitChan)

savedSnapshot, err := snapshotStore.Save(snapshot.Height, snapshot.Format, chunks)
if err != nil {
cmd.Println("failed to save snapshot", err)
return
}
quitChan <- savedSnapshot
}()

Check notice

Code scanning / CodeQL

Spawning a Go routine

Spawning a Go routine may be a possible source of non-determinism
@evan-forbes
Copy link
Member Author

called make proto-check-deps which downloaded the most recent stuff, which caused the proto breakage

@evan-forbes
Copy link
Member Author

evan-forbes commented Jun 9, 2023

hmm apparently we can't sync mocha-2 using this because of

7:29AM ERR Rejected proposal block: err="failed to load state at height 0; no commit info found (latest height: 0): invalid request" proposer="�6�\v��0®����e\x14\x02��E�" reason="failure to load query context"

looking into this now, but we can still merge this PR as is and patch whatever is needed after

Comment on lines -94 to -95
uint64 original_square_size = 5;
bytes hash = 6;

Choose a reason for hiding this comment

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

We still have square size and hash in our fork. I'm not sure if this is used though

Copy link
Member Author

Choose a reason for hiding this comment

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

👀 whoops might have forgot to update the link in the makefile. same tho I'm not sure we use it

// https://github.com/celestiaorg/celestia-specs/blob/master/specs/data_structures.md#availabledataheader
// Note that currently we list row and column roots in separate fields
// (different from the spec).
message DataAvailabilityHeader {

Choose a reason for hiding this comment

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

We don't need this because we import it directly from core right?

Copy link
Member Author

Choose a reason for hiding this comment

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

we just don't need this period for the sdk. I also believe its in the app now

@cmwaters
Copy link

cmwaters commented Jun 12, 2023

hmm apparently we can't sync mocha-2 using this because of

7:29AM ERR Rejected proposal block: err="failed to load state at height 0; no commit info found (latest height: 0): invalid request" proposer="�6�\v��0®����e\x14\x02��E�" reason="failure to load query context"

looking into this now, but we can still merge this PR as is and patch whatever is needed after

We haven't added prepareProposalState and processProposalState yet right?

I assume this comes from running process proposal on the first block. In our CometBFT fork we run ProcessProposal in blocksync however mainline CometBFT doesn't actually do this.

I agree that we should just merge this change as it is now and then follow up in a separate PR (or maybe #326) addresses it

@evan-forbes
Copy link
Member Author

evan-forbes commented Jun 12, 2023

We haven't added prepareProposalState and processProposalState yet right?
I assume this comes from running process proposal on the first block. In our CometBFT fork we run ProcessProposal in blocksync however mainline CometBFT doesn't actually do this.

#326 addresses a portion of this by replicating what is done in upstream v0.47.x, which is branching the state directly from the commit multi store to create a context which is passed to the new ABCI methods. However, this still doesn't fix being able to access that state at height 1, so we still have the same issues as we had before 🙁 celestiaorg/celestia-app#1899

I'm still unsure of how upstream handles this with the baseapp refactor, since as far as state access goes for the Prepare/Process #326 is doing the exact same thing as upstream

we will likely have to change something else as the current fix is pretty hacky
https://github.com/celestiaorg/celestia-app/blob/958ba9bb65bc7540e1d62d56ffba20d2dce8ec6b/app/square_size.go#L17-L20

@cmwaters
Copy link

cmwaters commented Jun 12, 2023

But it looks like its trying to access the state at height 0.

Also as I said, in blocksync we are calling ProcessProposal but in CometBFT they don't call it (i.e. it might not be SDK but at the Tendermint level)

@evan-forbes
Copy link
Member Author

But it looks like its trying to access the state at height 0.
Also as I said, in blocksync we are calling ProcessProposal but in CometBFT they don't call it (i.e. it might not be SDK but at the Tendermint level)

yeah I remember that, the

7:29AM ERR Rejected proposal block: err="failed to load state at height 0;

part was caused by us not passing the correct height (1) during ProcessProposal, which is fixed by #326 since we're passing the header instead of relying on the header included in other baseapp state.

However, we're still having issues in Prepare/ProcessProposal for accessing state at height 1, which upstream has solved.

@evan-forbes evan-forbes merged commit 95b3fdc into release/v0.46.x-celestia Jun 13, 2023
@evan-forbes evan-forbes deleted the evan/bump-upstream-v0.46.13 branch June 13, 2023 13:24
@rootulp rootulp mentioned this pull request Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pull upstream changes from v0.46.13
9 participants