feat!: sync with mainline ABCI #1003

cmwaters · 2023-04-26T19:55:26Z

Description

This PR modifies the ABCI interface to match that of CometBFT v0.37 and v0.47 of the SDK. There is no utility to extracting out blobs into a separate field within Data.

To get past the problem of the data availabiilty hash and the square size, the last transaction returned by the celestia state machine should always be that square size and the second to last transaction should always be the data hash

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use
unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments

cmwaters · 2023-04-26T20:01:25Z

rpc/core/blocks.go

-func EncodeDataRootTuple(height uint64, dataRoot [32]byte, squareSize uint64) ([]byte, error) {
+func EncodeDataRootTuple(height uint64, dataRoot []byte) ([]byte, error) {


It seems like this previously relied on the square size but when I follow the link https://github.com/celestiaorg/quantum-gravity-bridge/blob/5edb5906766b63eb6b9c24314df554feb1e83506/src/DataRootTuple.sol#L8 it seems that only the height and data root are used. Is it possible to remove the square size here? cc @sweexordious

there is a PR for that: celestiaorg/blobstream-contracts#145

Opened this to maybe remove the square size:
celestiaorg/blobstream-contracts#154

@cmwaters square size can be removed. Do you want to do it in this PR? or I open a subsequent PR to remove it?

That's great. Let's do it in a subsequent PR

cmwaters · 2023-04-27T08:39:07Z

Converting this as a draft for now. It seems my original assumption that the SquareSize was no longer needed in the consensus node is wrong - we still need it as part of the QGB setup. Will need to think a little more about the design here

rootulp

No blocking feedback, overall I'm a fan of this!

abci/types/result.go

state/execution.go

cmwaters · 2023-06-15T08:49:15Z

Opening this PR back for review again. This is for after mainnet just to be clear. I think we might even want to create a new branch v0.37.x-celestia and merge this into that (or use main)

…cal/abci

rootulp · 2023-06-15T15:59:47Z

This is for after mainnet just to be clear.

Does anyone have permission to create a new milestone in this repo? It could benefit from a Post-mainnet milestone (similar to celestia-app).

rootulp · 2023-06-15T16:18:07Z

abci/types/application.go

@@ -98,11 +101,18 @@ func (BaseApplication) ApplySnapshotChunk(req RequestApplySnapshotChunk) Respons
 }

 func (BaseApplication) PrepareProposal(req RequestPrepareProposal) ResponsePrepareProposal {
-	return ResponsePrepareProposal{BlockData: req.BlockData}
+	squareSize := len(req.Txs)


[question] why is the squareSize set to the number of transactions? I think of these as two distinct values. For example, a block with 100 send transactions could have a square size of 4.

Yeah this is a good point. So I've made the decision here that I want celestia-core to be specific to celestia-app. Concretely, we extend the rules around PrepareProposal and ProcessProposal that there must always be two transactions returned and the last and second to last are the squareSize and dataHash respectively. It's obviously somewhat hacky but means we keep within the boundaries of ABCI and are able to use the SDK without requiring a fork.

If the rule isn't followed, consensus panics. I could have chosen to simply ignore the rules when an application sends back 0 txs (thus supporting all the test applications in Tendermint) but I felt better to actually enforce the rule thus I had to modify all the test applications including the BaseApplication.

The square size and hash here are placeholders. I could use the value 0 (which is invalid in the case of Celestia) instead. This should not be used outside of testing

[optional] Thanks for clarifying that they are placeholders. If they are expected to be overwritten by celestia-app, then proposal to do something like:

func (BaseApplication) PrepareProposal(req RequestPrepareProposal) ResponsePrepareProposal { placeholderDataHash := tmhash.Sum(nil) placeholderSquareSizeBytes := make([]byte, 8) binary.BigEndian.PutUint64(placeholderSquareSizeBytes, uint64(1)) // placeholderDataHash and placeholderSquareSizeBytes are expected to be // overwritten by celestia-app. req.Txs = append(req.Txs, placeholderDataHash, placeholderSquareSizeBytes) return ResponsePrepareProposal{Txs: req.Txs} }

abci/types/application.go

abci/types/result.go

rootulp · 2023-06-15T16:22:06Z

consensus/mempool_test.go

@@ -254,5 +254,13 @@ func (app *CounterApplication) Commit() abci.ResponseCommit {

 func (app *CounterApplication) PrepareProposal(
 	req abci.RequestPrepareProposal) abci.ResponsePrepareProposal {
-	return abci.ResponsePrepareProposal{BlockData: req.BlockData}
+	dataHash := types.ToTxs(req.Txs).Hash()
+	squareSize := len(req.Txs)


same question regarding the relationship between squareSize and req.Txs

[nit] can these placeholders look identical to PrepareProposal in abci/types/application.go

rootulp · 2023-06-15T16:23:55Z

proto/tendermint/abci/types.proto

-  tendermint.types.Data block_data = 1;
-  // If an application decides to populate block_data with extra information, they can not exceed this value.
-  int64 block_data_size = 2;
+  repeated bytes            txs                  = 2;


[question] can the protobuf field 2 be reused? Is it a breaking change if it is reused? Asking b/c prior to this PR field 2 was a in64 for block_data_size

Yes this is an important detail. AFAIK we can't simply reserve those numbers and slide all the field numbers down if we want to have compatibility with the mainline version (in this case the SDK would not be able to decode the ABCI request that celestia-core would send). Thus we need to break ABCI. I've thought about this a bit and I don't think breaking ABCI will affect the backwards compatibility promises we make of our state machine. PrepareProposal and ProcessProposal (and all the other methods should work the same for v1 and v2)

proto/tendermint/abci/types.proto

state/execution.go

rootulp · 2023-06-15T16:30:17Z

state/execution.go

+			/*
+				TODO: Include vote extensions information when implementing vote extensions.
+				VoteExtension:   []byte{},
+			*/


[uber nit] I think it's convention to use // for this type of comment

Go provides C-style /* */ block comments and C++-style // line comments. Line comments are the norm; block comments appear mostly as package comments, but are useful within an expression or to disable large swaths of code.

Source: https://go.dev/doc/effective_go

This is just copied from cometbft/cometbft but I don't mind changing it to //

Oh no worries, we can preserve /* style comments if that's what is upstream.

rootulp · 2023-06-16T18:38:27Z

abci/types/application.go

@@ -98,11 +101,18 @@ func (BaseApplication) ApplySnapshotChunk(req RequestApplySnapshotChunk) Respons
 }

 func (BaseApplication) PrepareProposal(req RequestPrepareProposal) ResponsePrepareProposal {
-	return ResponsePrepareProposal{BlockData: req.BlockData}
+	squareSize := len(req.Txs)


[optional] Thanks for clarifying that they are placeholders. If they are expected to be overwritten by celestia-app, then proposal to do something like:

func (BaseApplication) PrepareProposal(req RequestPrepareProposal) ResponsePrepareProposal { placeholderDataHash := tmhash.Sum(nil) placeholderSquareSizeBytes := make([]byte, 8) binary.BigEndian.PutUint64(placeholderSquareSizeBytes, uint64(1)) // placeholderDataHash and placeholderSquareSizeBytes are expected to be // overwritten by celestia-app. req.Txs = append(req.Txs, placeholderDataHash, placeholderSquareSizeBytes) return ResponsePrepareProposal{Txs: req.Txs} }

rootulp · 2023-06-16T18:39:46Z

consensus/mempool_test.go

@@ -254,5 +254,13 @@ func (app *CounterApplication) Commit() abci.ResponseCommit {

 func (app *CounterApplication) PrepareProposal(
 	req abci.RequestPrepareProposal) abci.ResponsePrepareProposal {
-	return abci.ResponsePrepareProposal{BlockData: req.BlockData}
+	dataHash := types.ToTxs(req.Txs).Hash()
+	squareSize := len(req.Txs)


[nit] can these placeholders look identical to PrepareProposal in abci/types/application.go

test/e2e/app/app.go

rootulp

Overall LGTM w/ one blocking comment

README.md

abci/types/application.go

rach-id

Concerning the PR description:

`SquareSize` is also no longer needed...

It is still needed for QGB. I guess the description needs to be updated

evan-forbes

do we also need to change the data root to use the second to last hash?

celestia-core/types/block.go

Lines 1025 to 1037 in c3ab251

    
           // Hash returns the hash of the data 
        
           func (data *Data) Hash() cmtbytes.HexBytes { 
        
           	if data == nil { 
        
           		return (Txs{}).Hash() 
        
           	} 
        
           	if data.hash == nil { 
        
           		data.hash = data.Txs.Hash() // NOTE: leaves of merkle tree are TxIDs 
        
           	} 
        
           	// this is the expected behavior where `data.hash` was set by celestia-app 
        
           	// in PrepareProposal 
        
           	return data.hash 
        
           }

otherwise LGTM, should be nice

evan-forbes · 2023-07-18T11:25:24Z

proto/tendermint/abci/types.proto

-enum EvidenceType {
+message ExtendedVoteInfo {
+  Validator validator         = 1 [(gogoproto.nullable) = false];
+  bool      signed_last_block = 2;
+  bytes     vote_extension    = 3;  // Reserved for future use
+}


cmwaters · 2023-07-18T15:36:48Z

It is still needed for QGB. I guess the description needs to be updated

Yes square size is still present. I will update the description

cmwaters · 2023-07-19T07:42:06Z

do we also need to change the data root to use the second to last hash?

No because the txs in the data struct don't actually contain the hash. The hash (as well as the square size) remains part of the Data struct so the txs in their are simply the transactions to be committed.

The "hack" with the two extra transactions is purely across the ABCI layer

feat!: sync with mainline ABCI

c863bd9

cmwaters requested a review from evan-forbes as a code owner April 26, 2023 19:55

cmwaters commented Apr 26, 2023

View reviewed changes

cmwaters marked this pull request as draft April 27, 2023 08:13

cmwaters self-assigned this Apr 27, 2023

rootulp reviewed Apr 27, 2023

View reviewed changes

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

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

state/execution.go Outdated Show resolved Hide resolved

cmwaters added 4 commits June 14, 2023 14:59

Merge branch 'v0.34.x-celestia' into cal/abci

6ee16a7

add square size to data returned from prepare proposal

84da1ba

update readme documenting decision

444b2f3

fix bug

b91aaea

cmwaters marked this pull request as ready for review June 15, 2023 08:33

Merge branch 'v0.34.x-celestia' into cal/abci

69d968f

cmwaters added 2 commits June 15, 2023 12:00

remove reference of rocksdb

442d1a8

Merge branch 'cal/abci' of github.com:celestiaorg/celestia-core into …

cee4c8a

…cal/abci

rootulp reviewed Jun 15, 2023

View reviewed changes

cmwaters added this to the Post-mainnet milestone Jun 16, 2023

cmwaters added 2 commits June 16, 2023 12:30

apply suggestions

499c9a4

lint

842a367

rootulp previously approved these changes Jun 16, 2023

View reviewed changes

simplify base application and other implementations

6a6a610

cmwaters dismissed rootulp’s stale review via 6a6a610 June 19, 2023 15:00

cmwaters changed the base branch from v0.34.x-celestia to main June 21, 2023 12:51

rootulp reviewed Jun 26, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

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

incorporate suggestions

c892ebb

rach-id reviewed Jul 18, 2023

View reviewed changes

evan-forbes reviewed Jul 18, 2023

View reviewed changes

rach-id mentioned this pull request Jul 18, 2023

Remove the square size from the DataRootTuple celestiaorg/blobstream-contracts#154

Closed

rootulp approved these changes Jul 18, 2023

View reviewed changes

Merge branch 'main' into cal/abci

6035fba

rach-id mentioned this pull request Jul 20, 2023

Remove square size from data root tuple root generation #1040

Closed

evan-forbes approved these changes Jul 20, 2023

View reviewed changes

cmwaters merged commit 2c6cde4 into main Jul 20, 2023
33 checks passed

cmwaters deleted the cal/abci branch July 20, 2023 10:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: sync with mainline ABCI #1003

feat!: sync with mainline ABCI #1003

cmwaters commented Apr 26, 2023 •

edited

Loading

cmwaters Apr 26, 2023

rach-id Apr 26, 2023

rach-id Jul 18, 2023

rach-id Jul 20, 2023

cmwaters Jul 20, 2023

rach-id Jul 20, 2023

cmwaters commented Apr 27, 2023

rootulp left a comment

cmwaters commented Jun 15, 2023

rootulp commented Jun 15, 2023

rootulp Jun 15, 2023

cmwaters Jun 16, 2023

rootulp Jun 16, 2023

rootulp Jun 15, 2023

rootulp Jun 16, 2023

rootulp Jun 15, 2023

cmwaters Jun 16, 2023

rootulp Jun 15, 2023

cmwaters Jun 16, 2023

rootulp Jul 18, 2023

rootulp Jun 16, 2023

rootulp Jun 16, 2023

rootulp left a comment

rach-id left a comment

evan-forbes left a comment

evan-forbes Jul 18, 2023

cmwaters commented Jul 18, 2023

cmwaters commented Jul 19, 2023

		func EncodeDataRootTuple(height uint64, dataRoot [32]byte, squareSize uint64) ([]byte, error) {
		func EncodeDataRootTuple(height uint64, dataRoot []byte) ([]byte, error) {

	// Hash returns the hash of the data
	func (data *Data) Hash() cmtbytes.HexBytes {
	if data == nil {
	return (Txs{}).Hash()
	}
	if data.hash == nil {
	data.hash = data.Txs.Hash() // NOTE: leaves of merkle tree are TxIDs
	}

	// this is the expected behavior where `data.hash` was set by celestia-app
	// in PrepareProposal
	return data.hash
	}

feat!: sync with mainline ABCI #1003

feat!: sync with mainline ABCI #1003

Conversation

cmwaters commented Apr 26, 2023 • edited Loading

Description

PR checklist

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

cmwaters commented Apr 27, 2023

rootulp left a comment

Choose a reason for hiding this comment

cmwaters commented Jun 15, 2023

rootulp commented Jun 15, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

rach-id left a comment

Choose a reason for hiding this comment

evan-forbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmwaters commented Jul 18, 2023

cmwaters commented Jul 19, 2023

cmwaters commented Apr 26, 2023 •

edited

Loading