Custom Block HTR #5219

nisdas · 2020-03-26T07:11:30Z

Adds a custom block hash tree root method.
Add a custom hash tree root method for block bodies.
Add basic sanity tests.
Add benchmarks between the custom method and generic ssz method.
Gate this behind feature flag.

codecov · 2020-03-26T11:57:21Z

Codecov Report

❗ No coverage uploaded for pull request base (master@4118fa5). Click here to learn what that means.
The diff coverage is 13.82%.

@@            Coverage Diff            @@
##             master    #5219   +/-   ##
=========================================
  Coverage          ?   56.13%           
=========================================
  Files             ?      289           
  Lines             ?    22590           
  Branches          ?        0           
=========================================
  Hits              ?    12682           
  Misses            ?     8131           
  Partials          ?     1777

beacon-chain/rpc/beacon/blocks.go

beacon-chain/state/stateutil/blocks.go

terencechain

Looks good to me. Couple feedback
1.) Can you post the benchmark result?
2.) A few comments on clarity of naming
3.) A bit odd Custom block HTR and attestation HTR is under /state/stateutil pkg. Seems like a better place for them will be under /block/blockutil

beacon-chain/blockchain/process_block.go

beacon-chain/blockchain/receive_block.go

terencechain · 2020-03-26T13:40:42Z

beacon-chain/state/stateutil/attestations.go

+	return bitwiseMerkleizeArrays(fieldRoots, uint64(len(fieldRoots)), uint64(len(fieldRoots)))
+}
+
+func blockAttestationRoot(atts []*ethpb.Attestation) ([32]byte, error) {


Prefer naming it attestationsRoot as it doesn't need to be constrained with block, the function can be extensible to any object that needs a hash tree root of a list of attestations

we use the max block attestation capacity here, so this will only apply for beacon block attestations. If we use attestations which are larger than 128 in length, this root will be incorrect.

makes sense 👍

terencechain · 2020-03-26T13:44:30Z

beacon-chain/state/stateutil/blocks.go

@@ -32,6 +33,88 @@ func BlockHeaderRoot(header *ethpb.BeaconBlockHeader) ([32]byte, error) {
 	return bitwiseMerkleize(fieldRoots, uint64(len(fieldRoots)), uint64(len(fieldRoots)))
 }

+// BlockRoot returns the block hash tree root of the provided block.
+func BlockRoot(blk *ethpb.BeaconBlock) ([32]byte, error) {


I'm a bit torn on the naming. BlockRoot sounds a pure getter and its retrieving the the root within the block with no real work. Should we be consistent with HashTreeRootState and name it HashTreeRootBlock ? This applies to attestations as well

I was trying to put int more in line to how naming was done in this package, all the function names are basically data_structureRoot. Maybe we need to standardize naming here, although that will have to be in another pr

Are you saying HashTreeRootState should be named StateRoot?

I'm ok with that, as long as the 2 are consistent

Co-Authored-By: terence tsao <terence@prysmaticlabs.com>

nisdas · 2020-03-26T14:12:24Z

@terencechain
for

The benchmark results were the same for both ssz and custom ssz, this is explained by the ssz cache used in our generic ssz library and that we basically have the same attestation , 128 times in the block. I was unable to use the testutil to generate unique attestations so that is why the benchmark isn't wholly correct. I haven't found a way to switch off the cache in go-ssz
Agree with that , however the methods created in this PR basically use a lot of things from the

stateutils

package. Maybe we should refactor and put all these in /shared/sszutils or something.

terencechain · 2020-03-26T14:43:15Z

@terencechain
for

The benchmark results were the same for both ssz and custom ssz, this is explained by the ssz cache used in our generic ssz library and that we basically have the same attestation , 128 times in the block. I was unable to use the testutil to generate unique attestations so that is why the benchmark isn't wholly correct. I haven't found a way to switch off the cache in go-ssz

Agree with that , however the methods created in this PR basically use a lot of things from the
stateutils
package. Maybe we should refactor and put all these in /shared/sszutils or something.

Sounds good, do you mind opening an issue since we likely won't get to that this PR?

terencechain · 2020-03-26T14:54:46Z

beacon-chain/state/stateutil/attestations.go

+	fieldRoots := make([][32]byte, 3)
+
+	// Bitfield.
+	aggregationRoot, err := bitlistRoot(att.AggregationBits, 2048)


Instead of hard coding 2048, better to use params.BeaconConfig.MaxValidatorsPerCommittee

It changes to 1024 and it'll take next person a while to discover this is the issue :)

ah nice, catch. changing it now

…h-sharding into customBlockHTR

nisdas added 3 commits March 26, 2020 15:08

add custom htr

1e54ee2

fix root

a7f934b

fix everything

4aeafcd

nisdas marked this pull request as ready for review March 26, 2020 11:58

nisdas added the Ready For Review A pull request ready for code review label Mar 26, 2020

nisdas commented Mar 26, 2020

View reviewed changes

beacon-chain/rpc/beacon/blocks.go Outdated Show resolved Hide resolved

Apply suggestions from code review

b9ed31c

nisdas commented Mar 26, 2020

View reviewed changes

beacon-chain/state/stateutil/blocks.go Outdated Show resolved Hide resolved

Update beacon-chain/state/stateutil/blocks.go

2e3dfcd

terencechain reviewed Mar 26, 2020

View reviewed changes

nisdas and others added 2 commits March 26, 2020 22:08

Update beacon-chain/blockchain/receive_block.go

96b6f11

Co-Authored-By: terence tsao <terence@prysmaticlabs.com>

Update beacon-chain/blockchain/process_block.go

3f3c2c8

Co-Authored-By: terence tsao <terence@prysmaticlabs.com>

nisdas mentioned this pull request Mar 26, 2020

Refactor Stateutils to a Shared Library #5222

Closed

terencechain previously approved these changes Mar 26, 2020

View reviewed changes

terencechain reviewed Mar 26, 2020

View reviewed changes

nisdas added 2 commits March 26, 2020 23:06

terence's review

5b53ff4

Merge branch 'customBlockHTR' of https://github.com/prysmaticlabs/get…

8575b31

…h-sharding into customBlockHTR

nisdas dismissed terencechain’s stale review via 8575b31 March 26, 2020 15:07

terencechain approved these changes Mar 26, 2020

View reviewed changes

prylabs-bulldozer bot added 2 commits March 26, 2020 15:37

Merge refs/heads/master into customBlockHTR

bf06c6d

Merge refs/heads/master into customBlockHTR

686c0f8

rauljordan merged commit cdac3d6 into master Mar 26, 2020

rauljordan deleted the customBlockHTR branch March 26, 2020 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom Block HTR #5219

Custom Block HTR #5219

nisdas commented Mar 26, 2020 •

edited

codecov bot commented Mar 26, 2020 •

edited

terencechain left a comment

terencechain Mar 26, 2020

nisdas Mar 26, 2020

terencechain Mar 26, 2020

terencechain Mar 26, 2020

nisdas Mar 26, 2020

terencechain Mar 26, 2020

nisdas commented Mar 26, 2020

terencechain commented Mar 26, 2020

terencechain Mar 26, 2020

nisdas Mar 26, 2020

Custom Block HTR #5219

Custom Block HTR #5219

Conversation

nisdas commented Mar 26, 2020 • edited

codecov bot commented Mar 26, 2020 • edited

Codecov Report

terencechain 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

nisdas commented Mar 26, 2020

terencechain commented Mar 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nisdas commented Mar 26, 2020 •

edited

codecov bot commented Mar 26, 2020 •

edited