StateProofs: New block header field - SHA256 merkle root of the transactions #3829

Aharonee · 2022-03-24T09:03:04Z

Summary

Currently, the TxnRoot block header contains the root of the merkle tree built from the transactions in the block, using the SHA512_256 hash function.
Since the Ethereum VM (and others) does not support SHA512_256 natively, we have added a new header, which will be used by the Light Clients deployed on other networks in order to verify Algorand blocks.

config/consensus.go

codecov-commenter · 2022-03-27T09:00:41Z

Codecov Report

Merging #3829 (396fc3b) into master (5925aff) will increase coverage by 0.01%.
The diff coverage is 69.93%.

@@            Coverage Diff             @@
##           master    #3829      +/-   ##
==========================================
+ Coverage   49.80%   49.81%   +0.01%     
==========================================
  Files         409      409              
  Lines       68929    69145     +216     
==========================================
+ Hits        34332    34448     +116     
- Misses      30891    30981      +90     
- Partials     3706     3716      +10

Impacted Files	Coverage Δ
daemon/algod/api/server/v1/handlers/handlers.go	`0.63% <0.00%> (ø)`
daemon/algod/api/server/v2/handlers.go	`0.00% <0.00%> (ø)`
data/bookkeeping/genesis.go	`0.00% <0.00%> (ø)`
data/transactions/signedtxn.go	`29.62% <0.00%> (-3.71%)`	⬇️
data/transactions/transaction.go	`35.80% <0.00%> (-0.35%)`	⬇️
libgoal/libgoal.go	`2.78% <0.00%> (ø)`
netdeploy/remote/deployedNetwork.go	`19.77% <0.00%> (ø)`
crypto/hashes.go	`56.09% <33.33%> (-3.91%)`	⬇️
ledger/internal/eval.go	`67.28% <33.33%> (ø)`
data/bookkeeping/block.go	`56.61% <65.21%> (+1.37%)`	⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5925aff...396fc3b. Read the comment docs.

id-ms · 2022-03-27T09:24:26Z

data/bookkeeping/block.go

@@ -539,6 +560,31 @@ func (block Block) paysetCommit(t config.PaysetCommitType) (crypto.Digest, error
 	}
 }

+func (block Block) paysetCommitSHA256() (crypto.Digest, error) {
+	params, ok := config.Consensus[block.CurrentProtocol]


I think that this condition is already checked in the caller function.

I still need the params to verify that EnableSHA256TxnRootHeader is on, just ignoring the ok return value seems like bad practice.

Actually I think I'll just move the EnableSHA256TxnRootHeader check to the caller as well instead

id-ms · 2022-03-27T09:35:50Z

data/bookkeeping/block.go

+	if err != nil {
+		return crypto.Digest{}, err
+	}
+	// in case there are no leaves (e.g empty block with 0 txns) the merkle root is a slice with length of 0.


Since we use VC. the root will never be empty (even if there are no txn in the blocks).
This is due to the fact that we pad the VC to a full tree.

data/bookkeeping/txn_merkle.go

id-ms

overall looks quite good!
apart from some minor changes, I would recommend 4 tests
1- e2e test: run the network in the current version and make sure that TxnRoot.DigestSha256 is zero.
2-e2e test: run the network in the future version and make sure that TxnRoot.DigestSha256 is not zero.
3- a unit test that verifies that the catchpoint service makes sure that TxnRoot.DigestSha256 is present/not present according to the version.
4- a unit test that verifies that the evaluator makes sure that TxnRoot.DigestSha256 is present/not present according to the version.

data/bookkeeping/block.go

config/consensus.go

daemon/algod/api/client/restClient.go

daemon/algod/api/server/v2/handlers.go

data/bookkeeping/block.go

test/e2e-go/features/transactions/proof_test.go

daemon/algod/api/server/v2/handlers.go

algorandskiy · 2022-04-13T22:12:51Z

data/bookkeeping/block.go

@@ -131,6 +131,13 @@ type (
 		ParticipationUpdates
 	}

+	// TxnRoot represents the root of the merkle tree generated from the transaction in this block.
+	TxnRoot struct {


wonder if we can somehow write a test to ensure the old serialized and the new formats match

I can use an instance of old serialized BlockHeader hardcoded in some test and make sure it can be de-serialized correctly into the updated struct.

data/bookkeeping/block_test.go

test/e2e-go/features/transactions/proof_test.go

Aharonee · 2022-04-20T14:13:34Z

Update: since the TXID is generated using SHA512_256, using this as a leaf for transactions tree using SHA256 is useless, as you cannot verify that that transaction content is related to this specific TXID.
I've added support for SHA256 TXID for this specific use case.

id-ms · 2022-04-24T09:02:09Z

daemon/algod/api/algod.oas3.yml

@@ -630,7 +622,6 @@
                }
              },
              "required": [
-                "hashtype",
                "idx",
                "proof",
                "stibhash",


should we add a comment that guides the client to interpret the proof field as a concatenated array in which every element is 32-bytes?

We could but I don't think it's specifically related to this PR, or this particular change. We didn't change that aspect of the proof.

daemon/algod/api/server/v2/test/handlers_test.go

data/bookkeeping/block_test.go

id-ms · 2022-04-24T09:29:55Z

data/bookkeeping/txn_merkle.go

+// block, along with their ApplyData, as a Merkle tree vector commitment, using SHA256. This allows the
+// caller to either extract the root hash (for inclusion in the block
+// header), or to generate proofs of membership for transactions that are
+// in this block.


should we add here that we use the sha256 also for TXID and for signedTransactionInBlock?

That's internal logic I'm not sure it's relevant here

data/transactions/signedtxn.go

data/transactions/transaction.go

test/e2e-go/features/transactions/proof_test.go

id-ms · 2022-04-24T10:43:12Z

test/e2e-go/features/transactions/proof_test.go

-		copy(d[:], proofconcat)
-		proof.Path = append(proof.Path, d[:])
-		proofconcat = proofconcat[len(d):]
+	generateProof := func(h crypto.HashType, prfRsp generated.ProofResponse) (p merklearray.Proof) {


why not just use a function?

It's only used by this test so why contaminate the namespace? also I think it's a bit easier to reason about since you can read the code in a sequential manner.

algorandskiy

LGTM, couple minor comments.
@winder / @AlgoStephenAkiki could you have a look at REST API changes?

algorandskiy · 2022-04-27T16:29:00Z

data/bookkeeping/block_test.go

+	partitiontest.PartitionTest(t)
+	a := require.New(t)
+
+	// This serialized block header was generated from V32 e2e test, using the old BlockHeader struct which contains only TxnRoot SHA512_256 value


could you maybe add the commands used to obtain this value?

I checked out the master branch and added encode + print in the middle of an e2e test to get a somewhat full BlockHeader.
If it breaks you can alway checkout the previous commit and look at the actual decoded struct, right?

yes, I understand. Just add couple pointers, like:

edit XYZ test

add the following (example)

blk, err := ledger.Block(10) require.NoError(t, err) fmt.Printf("%x\n", protocol.Encode(&blk))

This is good for the tests maintainability

The problem is we have to checkout an old commit to encode a BlockHeader, since I'm changing the struct in this PR. Is there a better way you can think of or should I just add some comment at to how to do that and maybe hint a commit hash?

data/bookkeeping/txn_merkle.go

…3691)

daemon/algod/api/server/v2/handlers.go

winder · 2022-04-28T14:09:38Z

daemon/algod/api/algod.oas2.json

+              "sha512_256",
+              "sha256"


Would merkle-tree or vector-commitment be better names here? That seems like it would be more useful/descriptive than the generic hashing function being used to represent these things.

Both are necessary to verify the proof, but the SHA512_256 not being supported natively on the EVM is the reason we've added this parameter, vector commitment is just an optimization of merkle tree that's a bit more secure cryptographically.

I think I understand. I thought this was switching between two different algorithms. But this is literally just using a different hash function while otherwise doing the same thing?

So it's actually both, the different hash function was a necessity, while the different algorithm is used since it it's a "better" implementation of merkle tree (it's not actually necessary but since it has been integrated it should almost always be used instead of a standard merkle tree).
Do you think it should be more explicit in the parameter?

"sha512_256_merkle", "sha256_vector"

winder

Minor changes requested.

Co-authored-by: Will Winder <[email protected]>

winder · 2022-05-03T21:38:07Z

data/bookkeeping/block.go

+		DigestSha256     crypto.Digest `codec:"txn256"` // root of transaction vector commitment merkle tree using SHA256 hash function
+		DigestSha512_256 crypto.Digest `codec:"txn"`    // root of transaction merkle tree using SHA512_256 hash function


These names don't seem helpful to me. DigestSha256 and DigestSha512_256 are implementation details.

Previously the name was simply TxnRoot. The name says nothing about the data except for what it represents. With the new names, there is nothing about what the data represents, just how it was implemented.

This is similar to the conversation we had about the REST APIs. I think this would be significantly clearer if the variables were named MerkleTree and VectorCommitment.

@algorandskiy I'm curious what you think about this too.

I agree, MerkleTree or MerkleTreeTxnRoot sounds better

I'm afraid it is more complicated than that. we need to remember that TxnRoot might also represent a flat commitment (i.e hashing the concatenation of all the paysets in the block). That was the implementation on earlier protocol versions. Therefore I'm not sure using the word merkleTree or VectorCommitment is good.

so we might want to change the structure name to TxnCommitment or similar, but those fields above are just the result of hash and cannot really state what was the input

# Conflicts: # config/consensus.go

winder · 2022-05-10T16:52:41Z

daemon/algod/api/server/v2/handlers.go

@@ -574,6 +575,10 @@ func (v2 *Handlers) GetProof(ctx echo.Context, round uint64, txid string, params
 		return badRequest(ctx, err, errNoTxnSpecified, v2.Log)
 	}

+	if params.Hashtype != nil && *params.Hashtype != "sha512_256" && *params.Hashtype != "sha256" {


nit: UnmarshalHashType and/or Sha512_256.String() / Sha256.String() can be used to simplify some of these checks and leverage the compiler to validate these strings.

winder

LGTM

cce · 2022-05-13T17:08:07Z

daemon/algod/api/server/v2/handlers.go

-				Stibhash:  stibhash[:],
-				Idx:       uint64(idx),
-				Treedepth: uint64(proof.TreeDepth),
-				Hashtype:  proof.HashFactory.HashType.String(),


So you removed hashtype from the response, but it is still listed as required in the ProofResponse YAML spec...

This should ensure make msgp has been run for changes that impact msgp serialization on CI builds. It looks like #3829 was merged but missed changes to agreement/msgp_gen.go and gci updates from algorand/msgp#14 were not incorporated into #3919.

…actions (algorand#3829) Currently, the TxnRoot block header contains the root of the merkle tree built from the transactions in the block, using the SHA512_256 hash function. Since the Ethereum VM (and others) does not support SHA512_256 natively, we have added a new header, which will be used by the Light Clients deployed on other networks in order to verify Algorand blocks. Co-authored-by: algoidan <[email protected]>

This should ensure make msgp has been run for changes that impact msgp serialization on CI builds. It looks like algorand#3829 was merged but missed changes to agreement/msgp_gen.go and gci updates from algorand/msgp#14 were not incorporated into algorand#3919.

Aharonee added the Enhancement label Mar 24, 2022

Aharonee changed the title ~~Crypto: New block header field - SHA256 merkle root of the transactions~~ StateProofs: New block header field - SHA256 merkle root of the transactions Mar 24, 2022

Aharonee force-pushed the SHA256-Block-Header branch from 30b5a09 to 3ca179a Compare March 27, 2022 08:19

id-ms reviewed Mar 27, 2022

View reviewed changes

config/consensus.go Show resolved Hide resolved

id-ms reviewed Mar 27, 2022

View reviewed changes

data/bookkeeping/txn_merkle.go Show resolved Hide resolved

id-ms reviewed Mar 27, 2022

View reviewed changes

data/bookkeeping/block.go Outdated Show resolved Hide resolved

Aharonee marked this pull request as ready for review March 29, 2022 13:57

id-ms reviewed Mar 29, 2022

View reviewed changes

id-ms reviewed Mar 30, 2022

View reviewed changes

test/e2e-go/features/transactions/proof_test.go Outdated Show resolved Hide resolved

Aharonee assigned cce and unassigned cce Mar 31, 2022

Aharonee requested review from cce and algorandskiy March 31, 2022 10:44

Aharonee self-assigned this Mar 31, 2022

algorandskiy reviewed Apr 13, 2022

View reviewed changes

id-ms reviewed Apr 24, 2022

View reviewed changes

Aharonee force-pushed the SHA256-Block-Header branch 2 times, most recently from 4263d31 to 323bba4 Compare April 25, 2022 08:05

algorandskiy reviewed Apr 27, 2022

View reviewed changes

Aharonee added 5 commits April 28, 2022 15:36

[CHERRY-PICK] Algod: add support for SHA256 in merkle tree (algorand#…

528d0f8

…3691)

Neww block header: SHA256 merkle root of the transactions

8066623

rename fields

1345f0b

Update GetProof API endpoint and add parameter

4e2c22c

refactor

8647817

winder reviewed Apr 28, 2022

View reviewed changes

daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved

winder reviewed Apr 28, 2022

View reviewed changes

winder suggested changes Apr 28, 2022

View reviewed changes

Aharonee and others added 3 commits April 28, 2022 17:46

Update daemon/algod/api/server/v2/handlers.go

d736fba

Co-authored-by: Will Winder <[email protected]>

Update daemon/algod/api/server/v2/handlers.go

1e76b89

Co-authored-by: Will Winder <[email protected]>

return hashtype in ProofResponse

ab275b0

winder reviewed May 3, 2022

View reviewed changes

Aharonee added 6 commits May 8, 2022 11:31

Merge branch 'master' into SHA256-Block-Header

b3d6bea

# Conflicts: # config/consensus.go

validate hashtype in GetProof

430154e

Merge branch 'master' into SHA256-Block-Header

b0eafe4

rename TxnRoot

04814e1

rename consensus flag

bfd0c51

make generate

396fc3b

winder reviewed May 10, 2022

View reviewed changes

winder approved these changes May 10, 2022

View reviewed changes

algorandskiy approved these changes May 10, 2022

View reviewed changes

algorandskiy merged commit bd18d04 into algorand:master May 10, 2022

cce mentioned this pull request May 11, 2022

CI: ensure msgp generator has been run and is clean #3978

Merged

cce reviewed May 13, 2022

View reviewed changes

cce mentioned this pull request May 26, 2022

StateProofs: fix GetProof endpoint response #3985

Merged

This was referenced May 27, 2022

[Not Merged] go-algorand 3.7.0-beta Release PR #4049

Closed

Release: go-algorand 3.7.0-beta Release PR #4050

Merged

Algo-devops-service mentioned this pull request Jun 3, 2022

go-algorand 3.7.0-stable Release PR #4071

Merged

This was referenced Jun 8, 2022

go-algorand 3.7.1-stable Release PR #4091

Merged

go-algorand 3.7.2-stable Release PR #4118

Merged

algonoah mentioned this pull request Aug 10, 2022

go-algorand 3.9.0-beta Release PR #4383

Merged

onetechnical mentioned this pull request Sep 1, 2022

go-algorand 3.9.2-stable Release PR #4506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StateProofs: New block header field - SHA256 merkle root of the transactions #3829

StateProofs: New block header field - SHA256 merkle root of the transactions #3829

Aharonee commented Mar 24, 2022 •

edited

Loading

codecov-commenter commented Mar 27, 2022 •

edited

Loading

id-ms Mar 27, 2022

Aharonee Mar 28, 2022

Aharonee Mar 28, 2022

id-ms Mar 27, 2022

id-ms left a comment

algorandskiy Apr 13, 2022

Aharonee Apr 19, 2022

Aharonee commented Apr 20, 2022

id-ms Apr 24, 2022

Aharonee Apr 24, 2022

id-ms Apr 24, 2022

Aharonee Apr 24, 2022

id-ms Apr 24, 2022

Aharonee Apr 25, 2022

algorandskiy left a comment

algorandskiy Apr 27, 2022

Aharonee Apr 28, 2022

algorandskiy Apr 28, 2022

Aharonee Apr 28, 2022

winder Apr 28, 2022

Aharonee Apr 28, 2022

winder Apr 28, 2022

Aharonee Apr 28, 2022

winder left a comment

winder May 3, 2022

algorandskiy May 3, 2022

id-ms May 4, 2022 •

edited

Loading

winder May 10, 2022 •

edited

Loading

winder left a comment

cce May 13, 2022

Aharonee May 15, 2022

		DigestSha256 crypto.Digest `codec:"txn256"` // root of transaction vector commitment merkle tree using SHA256 hash function
		DigestSha512_256 crypto.Digest `codec:"txn"` // root of transaction merkle tree using SHA512_256 hash function

StateProofs: New block header field - SHA256 merkle root of the transactions #3829

StateProofs: New block header field - SHA256 merkle root of the transactions #3829

Conversation

Aharonee commented Mar 24, 2022 • edited Loading

Summary

codecov-commenter commented Mar 27, 2022 • edited Loading

Codecov Report

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

id-ms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aharonee commented Apr 20, 2022

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

algorandskiy left a 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

winder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

id-ms May 4, 2022 • edited Loading

Choose a reason for hiding this comment

winder May 10, 2022 • edited Loading

Choose a reason for hiding this comment

winder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aharonee commented Mar 24, 2022 •

edited

Loading

codecov-commenter commented Mar 27, 2022 •

edited

Loading

id-ms May 4, 2022 •

edited

Loading

winder May 10, 2022 •

edited

Loading