abci: Move `app_hash` parameter from `Commit` to `FinalizeBlock` by sergio-mena · Pull Request #8664 · tendermint/tendermint

sergio-mena · 2022-06-01T07:32:17Z

This PR doesn't address the spec as there is a parallel, ongoing one dealing with FinalizeBlock/ Commit all over the spec (see #7659).

All UTs are passing, except TestRootConfig, which is failing also on the baseline

e2e are passing:

make -j2 docker generator runner tests && ./build/generator -g 4 -d networks/nightly/ && ./run-multiple.sh networks/nightly/*-group00-*.toml

tychoish · 2022-06-01T08:22:21Z

abci/types/types.go

 // IsErr returns true if Code is something other than OK.
 func (r ResponseQuery) IsErr() bool {
 	return r.Code != CodeTypeOK
+


this is probably unintentional.

yes, thanks for spotting

tychoish · 2022-06-01T08:22:55Z

internal/consensus/mempool_test.go

 		txBytes := make([]byte, 8)
 		binary.BigEndian.PutUint64(txBytes, uint64(i))
-		err := assertMempool(t, cs.txNotifier).CheckTx(ctx, txBytes, nil, mempool.TxInfo{})
+		var rCode uint32


this is always 0? maybe best to omit

It is filled by the functor I added to the CheckTx call in line 141.
This was a problem in this test: CheckTx was returning "no error", but the Code field in the CheckTx response was being ignored by the test... so CheckTx was failing but the TC was thinking it was passing.
So I was chasing a problem, and it was really cumbersome before adding line 143.

on line 141 you reference r.Code not rCode, so I still think this isn’t being used, though perhaps not intentionally.

I'm setting rCode (captured by closure) to the value of r.Code

tychoish · 2022-06-01T08:24:47Z

in your e2e command-line above, you only run 25% of the e2e tests. Is there a reason for this?

abci/types/types.go

cmwaters

Few comments but mostly looks good. This will definitely need a changelog and a note in UPGRADING.md (the latter can be done altogether at a separate time if that's more convenient)

cmwaters · 2022-06-01T09:25:01Z

internal/state/execution.go

 		"height", block.Height,
 		"num_txs", len(block.Txs),
-		"app_hash", fmt.Sprintf("%X", res.Data),
+		"app_hash", appHash,


We could probably remove that log here and just have it in FinalizeBlock

Two things:

I just realize I removed the formatting. That's probably bad. Will put it back

To your question. I thought about it, and decided to pass the hash to this function just to print it here, under the assumption that this is an important log people are used to seeing when troubleshooting. So didn't want to split it to minimize disruption. On the other hand, it might lead people to think the app_hash is still being returned at commit time.
So, I'm 50-50 on what to do... a little push from you in either direction and I will leave it or change it.

I would try to put it around FinalizeBlock if we can - to illustrate that it's returned there. We could also return the blockHash here which is associated with that height.

OK, will log on FinalizeBlock

test/e2e/app/state.go

cmwaters · 2022-06-01T09:31:37Z

proto/tendermint/abci/types.proto

@@ -332,7 +331,6 @@ message ResponseFinalizeBlock {
  repeated ValidatorUpdate         validator_updates       = 3 [(gogoproto.nullable) = false];
  tendermint.types.ConsensusParams consensus_param_updates = 4;
  bytes                            app_hash                = 5;


This is a subjective conversation about words so perhaps it won't mean anything, but do we want to change the terminology from app_hash to data. app_hash is probably what's more commonly used, but I like data because it doesn't assume a hash - but rather any arbitrary data that will be agreed upon

I've always found data "hard to grasp" when working on the spec (had to think for 10 secs what "data" this was referring to).
How about app_data ?

Honestly, I'm also happy with app_hash. I'll leave it up to your discretion

sergio-mena · 2022-06-01T09:33:02Z

in your e2e command-line above, you only run 25% of the e2e tests. Is there a reason for this?

e2e tests take a long time on M1s, so just copy-pasted the e2e-nightly-master, and ran the "group (00)". Found it a reasonable tradeoff.
Probably, the generation step could have been fine-tuned to only generate testnets I'm testing, but generation takes no time, and copy-pasting the command from CI was good enough.

tychoish · 2022-06-01T09:34:02Z

internal/consensus/mempool_test.go

 		txBytes := make([]byte, 8)
 		binary.BigEndian.PutUint64(txBytes, uint64(i))
-		err := assertMempool(t, cs.txNotifier).CheckTx(ctx, txBytes, nil, mempool.TxInfo{})
+		var rCode uint32


on line 141 you reference r.Code not rCode, so I still think this isn’t being used, though perhaps not intentionally.

tychoish · 2022-06-01T09:37:59Z

in your e2e command-line above, you only run 25% of the e2e tests. Is there a reason for this?

e2e tests take a long time on M1s, so just copy-pasted the e2e-nightly-master, and ran the "group (00)". Found it a reasonable tradeoff. Probably, the generation step could have been fine-tuned to only generate testnets I'm testing, but generation takes no time, and copy-pasting the command from CI was good enough.

you can run the nightly tests on your branch locally. I think its useful to run specific tests locally for debugging or smoke testing purposes, but it’s a better use of (everyone’s) time to run the nightly tests in CI for a branch in cases like this. The tests are sensitive enough to timing concerns that running them on a laptop may not be super useful.

internal/state/execution.go

Co-authored-by: Callum Waters <cmwaters19@gmail.com>

sergio-mena · 2022-06-01T09:47:59Z

This will definitely need a changelog and a note in UPGRADING.md

Will do now. Please double-check once I push

cmwaters · 2022-06-01T13:13:11Z

CHANGELOG_PENDING.md

  - [config] \#8222 default indexer configuration to null. (@creachadair)
  - [rpc] \#8570 rework timeouts to be per-method instead of global. (@creachadair)
  - [rpc] \#8624 deprecate `broadcast_tx_commit` and `braodcast_tx_sync` and `broadcast_tx_async` in favor of `braodcast_tx`. (@tychoish)
+  - [abci] [ABCI++](https://github.com/orgs/tendermint/projects/9) is not backwards compatible with ABCI. (@sergio-mena)


I think this is not necessary. If you want to mention the non-backwards compatibility, I would do it in the upgrading document

OK, removing...

cmwaters · 2022-06-01T13:25:32Z

UPGRADING.md

+#### Moving the `app_hash` parameter
+
+The Application's hash (or any data representing the Application's current
+state) is known by the time `FinalizeBlock` finishes its execution.
+Accordingly, the `app_hash` parameter has been moved from `ResponseCommit` to
+`ResponseFinalizeBlock`, since it makes sense for the Application to return
+this value as soon as is it known.


This in many ways should actually be covered by the ABCI++ spec and upgrade notes

Yes. We need to expand/improve this section, with selected information from the spec that is important for App developers (in case they choose not to read the spec).
I'll leave it as is for the moment, if you agree, and will come back to it when tackling doc in a holistic way.

@cmwaters

* [partial cherry-pick] abci: Move `app_hash` parameter from `Commit` to `FinalizeBlock` (tendermint#8664) * Removed from proto * make proto-gen * make build works * make some tests pass * Fix TestMempoolTxConcurrentWithCommit * Minor change * Update abci/types/types.go * Update internal/state/execution.go * Update test/e2e/app/state.go Co-authored-by: Callum Waters <cmwaters19@gmail.com> * Updated changelog and `UPGRADING.md` * Fixed abci-cli tests, and doc * Addressed @cmwaters' comments * Addressed @cmwaters' comments, part 2 Co-authored-by: Callum Waters <cmwaters19@gmail.com> * Levftover typo in spec --------- Co-authored-by: Callum Waters <cmwaters19@gmail.com>

yzang2019 · 2023-10-31T19:43:21Z

@sergio-mena Instead of moving apphash from Commit to FinalizeBlock, why not still keep the appHash in the Commit Response, but just add that for FinalizeBlock response?

We are using ABCI++ and we are pretty frustrated because the Commit interface doesn't really return the AppHash, so it forces us to implement the getWorkingHash to always return the finalized commit hash. In a case where people want to customize the state commit store, it is not very flexible and convenient than just using the hash returned from the Commit response.

Is there any plan to add the app hash back to to the commit response in the future? cc @alexanderbez

sergio-mena added 6 commits May 31, 2022 20:22

Removed from proto

07de113

make proto-gen

13b0e22

make build works

bd8c3da

make some tests pass

e46132c

Fix TestMempoolTxConcurrentWithCommit

e18a603

Minor change

a6b141c

sergio-mena requested review from ancazamfir, cmwaters, creachadair, ebuchman, jmalicevic, thanethomson, tychoish and williambanfield as code owners June 1, 2022 07:32

tychoish changed the title ~~Move app_hash parameter from Commit to FinalizeBlock~~ abci: Move app_hash parameter from Commit to FinalizeBlock Jun 1, 2022

tychoish approved these changes Jun 1, 2022

View reviewed changes

sergio-mena commented Jun 1, 2022

View reviewed changes

abci/types/types.go Outdated Show resolved Hide resolved

Update abci/types/types.go

901cb2b

cmwaters approved these changes Jun 1, 2022

View reviewed changes

tychoish approved these changes Jun 1, 2022

View reviewed changes

sergio-mena commented Jun 1, 2022

View reviewed changes

internal/state/execution.go Outdated Show resolved Hide resolved

sergio-mena and others added 2 commits June 1, 2022 11:42

Update internal/state/execution.go

d52e021

Update test/e2e/app/state.go

9661644

Co-authored-by: Callum Waters <cmwaters19@gmail.com>

Updated changelog and UPGRADING.md

1ecd19e

cmwaters reviewed Jun 1, 2022

View reviewed changes

sergio-mena added 4 commits June 1, 2022 15:30

Fixed abci-cli tests, and doc

c0da2fb

Addressed @cmwaters' comments

bc37f98

Addressed @cmwaters' comments, part 2

66e2beb

Merge remote-tracking branch 'origin/master' into sergio/move_app_hash

70eb88f

sergio-mena merged commit 56fc80d into master Jun 1, 2022

sergio-mena deleted the sergio/move_app_hash branch June 1, 2022 16:53

sergio-mena mentioned this pull request Aug 2, 2022

ABCI++ Prepare/Process Proposal #9053

Closed

35 tasks

sergio-mena mentioned this pull request Sep 8, 2022

ABCI++ vote extensions & FinalizeBlock #9396

Closed

17 tasks

sergio-mena mentioned this pull request Nov 21, 2022

abci: implement finalize block #9468

Merged

This was referenced Dec 19, 2022

ABCI 2.0: Final adjustments #9916

Closed

ABCI 2.0: Final adjustments cometbft/cometbft#10

Closed

sergio-mena mentioned this pull request Mar 7, 2023

Move apphash from Committo FinalizeBlock (leftovers) cometbft/cometbft#478

Merged

3 tasks

Conversation

sergio-mena commented Jun 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tychoish commented Jun 1, 2022

Uh oh!

Uh oh!

cmwaters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergio-mena Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergio-mena commented Jun 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tychoish commented Jun 1, 2022

Uh oh!

Uh oh!

sergio-mena commented Jun 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yzang2019 commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sergio-mena Jun 1, 2022 •

edited

Loading

yzang2019 commented Oct 31, 2023 •

edited

Loading