Fix epoch block marshaling #1688

piersy · 2021-09-14T21:28:19Z

Description

This is a continuation of the PR from @jpjamipark:

add custom marshal function for epochsnarkdata #1682

EpochSnarkData unmarshaling was broken.

RPCMarshalBlock was marshaling both the fields of EpochSnarkData as hexutil.Bytes.

Lines 1071 to 1076 in 43ae190

    
           epochSnarkData := block.EpochSnarkData() 
        
           if epochSnarkData != nil && !epochSnarkData.IsEmpty() { 
        
           	fields["epochSnarkData"] = map[string]interface{}{ 
        
           		"bitmap":    hexutil.Bytes(block.EpochSnarkData().Bitmap.Bytes()), 
        
           		"signature": hexutil.Bytes(block.EpochSnarkData().Signature), 
        
           	}

@jpjamipark Had mentioned that:

I was not able to get the gencodec library to automatically generate an UnmarshalJSON function that properly handle conversions from hexutil.Bytes to big.Int, so I modified gencodec generated code and added it to block.go instead.

I believe that it's actually not possible to use gencodec for this task because the manual conversion performed in ethapi/api.go when encoding the big.Int Bitmap field, means that we cannot use the field type override in gencodec, because you cannot mark a field of type big.Int to be serialized as hexutil.Bytes.

gencodec doc

Field types in the override struct must be trivially convertible to the original field type.

Unless we change the marshaling in internal/ethapi/api.go I don't think we can use gencodec for this task and since, changing the marshaling in internal/ethapi/api.go to use hexutil.Big actually changes the output bytes (see below) its probably not a good idea.

So in this case we take the approach of adding a manually specified marshal and unmarhsal json methods on EpochSnarkData.

package main

import (
	"fmt"
	"math/big"

	"github.com/celo-org/celo-blockchain/common/hexutil"
)

func main() {
	i := big.NewInt(0)
	for i.Uint64() < 50 {
		i.Add(i, big.NewInt(1))
		s, _ := hexutil.Bytes(i.Bytes()).MarshalText()
		s2, _ := hexutil.Big(*i).MarshalText()
		fmt.Printf("s %s : s2 %s\n", s, s2)
	}
}

// Output
s 0x01 : s2 0x1
s 0x02 : s2 0x2
s 0x03 : s2 0x3
s 0x04 : s2 0x4
s 0x05 : s2 0x5
s 0x06 : s2 0x6
s 0x07 : s2 0x7
s 0x08 : s2 0x8
s 0x09 : s2 0x9
s 0x0a : s2 0xa
s 0x0b : s2 0xb
s 0x0c : s2 0xc
s 0x0d : s2 0xd
s 0x0e : s2 0xe
s 0x0f : s2 0xf
s 0x10 : s2 0x10
s 0x11 : s2 0x11
s 0x12 : s2 0x12
s 0x13 : s2 0x13
s 0x14 : s2 0x14
s 0x15 : s2 0x15

Other changes

Added the ability to wait for blocks in test.Tracker, to support the test that waits for an epoch block.

Tested

Added a new test to check that the marshaling/unmarshaling works on epoch blocks.

Related issues

Fixes ethclient.Client can't unmarshal epoch blocks #1574

Backwards compatibility

No.

This breaks backwards compatibility, because programs using the geth client to decode epoch blocks will now decode the block instead of returning an error.

codecov · 2021-09-15T15:52:05Z

Codecov Report

Merging #1688 (37ed12d) into master (00a9a9d) will increase coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 37ed12d differs from pull request most recent head 3dda340. Consider uploading reports for the commit 3dda340 to get more accurate results

@@            Coverage Diff             @@
##           master    #1688      +/-   ##
==========================================
+ Coverage   56.03%   56.04%   +0.01%     
==========================================
  Files         606      606              
  Lines       80271    80291      +20     
==========================================
+ Hits        44980    45002      +22     
- Misses      31756    31758       +2     
+ Partials     3535     3531       -4

Impacted Files	Coverage Δ
core/types/block.go	`31.47% <0.00%> (-3.56%)`	⬇️
...nsensus/istanbul/core/roundstate_save_decorator.go	`91.37% <0.00%> (-3.45%)`	⬇️
p2p/enode/iter.go	`90.55% <0.00%> (-2.37%)`	⬇️
consensus/istanbul/core/commit.go	`66.44% <0.00%> (-0.66%)`	⬇️
core/tx_pool.go	`73.93% <0.00%> (-0.12%)`	⬇️
eth/downloader/downloader.go	`76.48% <0.00%> (-0.10%)`	⬇️
eth/fetcher/block_fetcher.go	`83.66% <0.00%> (+0.50%)`	⬆️
p2p/simulations/http.go	`70.65% <0.00%> (+0.54%)`	⬆️
rpc/client.go	`83.50% <0.00%> (+0.68%)`	⬆️
trie/proof.go	`76.40% <0.00%> (+0.74%)`	⬆️
... and 7 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 00a9a9d...3dda340. Read the comment docs.

Its now just called Tracker

Epoch blocks previously were not json unmarshalable by the geth client, they now are.

piersy · 2021-09-24T12:44:58Z

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit 37ed12d

coverage:  44.5% of statements across all listed packages
coverage:  54.1% of statements in consensus/istanbul
coverage:  45.0% of statements in consensus/istanbul/backend
coverage:   0.0% of statements in consensus/istanbul/backend/backendtest
coverage:  43.3% of statements in consensus/istanbul/backend/internal/db
coverage:  24.1% of statements in consensus/istanbul/backend/internal/enodes
coverage:  22.6% of statements in consensus/istanbul/backend/internal/replica
coverage:  63.4% of statements in consensus/istanbul/core
coverage:   0.0% of statements in consensus/istanbul/proxy
coverage:  75.3% of statements in consensus/istanbul/uptime
coverage: 100.0% of statements in consensus/istanbul/uptime/store
coverage:  51.8% of statements in consensus/istanbul/validator
coverage:  79.2% of statements in consensus/istanbul/validator/random

hbandura · 2021-09-24T18:45:06Z

core/types/block.go

+	if err := json.Unmarshal(input, &dec); err != nil {
+		return err
+	}
+	if dec.Bitmap != nil {


What should r.UnmarshalJSON() do if the received bitmap and signature are nil, but r holds some values for it? Should it:

keep the values or

overwrite the r values with the nil interpretation of it

I have a hunch that it should do 2, but I'm honestly asking.

Even so, this implementation seems that it does a mix. If bitmap is nil, it won't overwrite the value, but if signature is not, it would, while not returning any error.

Unmarshaler is the interface implemented by types that can unmarshal a JSON description of themselves. The input can be assumed to be a valid encoding of a JSON value. UnmarshalJSON must copy the JSON data if it wishes to retain the data after returning. By convention, to approximate the behavior of Unmarshal itself, Unmarshalers implement UnmarshalJSON([]byte("null")) as a no-op.

It is not clear what it should do, but I would veer towards something that in most cases would make (marshal(unmarshal(json)) be as idempotent as possible.

Or maybe I'm just overthinking this and it's not that important

You are right, its not that clear.

I think there's only one valid case for calling UnmarshalJson on non empty object, that would be to re-use a struct for unmarshaling to avoid re-allocating memory.

I think we should disallow code from setting some fields of a struct and expecting the rest to be set by unmarshaling, it seems like a fragile and confusing approach.

So to support that behavior, the unmarshal function would need to set all fields when executed. Interestingly though it looks like that implementation would violate the convention you quoted above, but I wrote a test on go playground to check this and it seems that Unmarshal will overwrite a struct with nil when decoding []byte("null") so it looks like its not a no-op.

https://play.golang.org/p/dHiWmj8X7KD

So based on that I think we can drop the nil checks.

Ok so I actually raised an issue on the golang repo because this was not clear to me golang/go#48646, but for now I still think dropping the nil checks is the best course of action.

piersy · 2021-09-27T19:45:36Z

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit 3dda340

coverage:  44.8% of statements across all listed packages
coverage:  54.1% of statements in consensus/istanbul
coverage:  45.1% of statements in consensus/istanbul/backend
coverage:   0.0% of statements in consensus/istanbul/backend/backendtest
coverage:  43.3% of statements in consensus/istanbul/backend/internal/db
coverage:  24.1% of statements in consensus/istanbul/backend/internal/enodes
coverage:  22.6% of statements in consensus/istanbul/backend/internal/replica
coverage:  64.2% of statements in consensus/istanbul/core
coverage:   0.0% of statements in consensus/istanbul/proxy
coverage:  75.3% of statements in consensus/istanbul/uptime
coverage: 100.0% of statements in consensus/istanbul/uptime/store
coverage:  51.8% of statements in consensus/istanbul/validator
coverage:  79.2% of statements in consensus/istanbul/validator/random

piersy requested review from hbandura and trianglesphere September 14, 2021 21:28

piersy requested a review from a team as a code owner September 14, 2021 21:28

piersy force-pushed the piersy/fix-epoch-block-marshaling branch from e4c1f9b to b1bdb23 Compare September 15, 2021 15:31

piersy added 3 commits September 17, 2021 09:18

Extend transaction tracker to track blocks as well

f1d2ad8

Its now just called Tracker

Rename transaction_tracker.go -> tracker.go

b848c70

Fix epoch block marshaling and add test for it

5393345

Epoch blocks previously were not json unmarshalable by the geth client, they now are.

piersy force-pushed the piersy/fix-epoch-block-marshaling branch from b1bdb23 to 5393345 Compare September 17, 2021 08:22

Merge branch 'master' into piersy/fix-epoch-block-marshaling

37ed12d

hbandura reviewed Sep 24, 2021

View reviewed changes

piersy requested a review from hbandura September 27, 2021 13:40

Don't skip nil fields when unmarshaling EpochSnarkData

3dda340

hbandura approved these changes Sep 27, 2021

View reviewed changes

piersy merged commit ae053df into master Sep 28, 2021

piersy deleted the piersy/fix-epoch-block-marshaling branch September 28, 2021 09:20

jpjamipark mentioned this pull request Sep 30, 2021

Unmarshalling error while fetching the block at height 311040 celo-org/kliento#16

Closed

piersy added this to the Reliability track milestone Oct 1, 2021

piersy self-assigned this Oct 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix epoch block marshaling #1688

Fix epoch block marshaling #1688

piersy commented Sep 14, 2021 •

edited

Loading

codecov bot commented Sep 15, 2021 •

edited

Loading

piersy commented Sep 24, 2021

hbandura Sep 24, 2021

hbandura Sep 24, 2021

piersy Sep 27, 2021 •

edited

Loading

piersy Sep 27, 2021 •

edited

Loading

piersy commented Sep 27, 2021

	epochSnarkData := block.EpochSnarkData()
	if epochSnarkData != nil && !epochSnarkData.IsEmpty() {
	fields["epochSnarkData"] = map[string]interface{}{
	"bitmap": hexutil.Bytes(block.EpochSnarkData().Bitmap.Bytes()),
	"signature": hexutil.Bytes(block.EpochSnarkData().Signature),
	}

Fix epoch block marshaling #1688

Fix epoch block marshaling #1688

Conversation

piersy commented Sep 14, 2021 • edited Loading

Description

Other changes

Tested

Related issues

Backwards compatibility

codecov bot commented Sep 15, 2021 • edited Loading

Codecov Report

piersy commented Sep 24, 2021

hbandura Sep 24, 2021

Choose a reason for hiding this comment

hbandura Sep 24, 2021

Choose a reason for hiding this comment

piersy Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

piersy Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

piersy commented Sep 27, 2021

piersy commented Sep 14, 2021 •

edited

Loading

codecov bot commented Sep 15, 2021 •

edited

Loading

piersy Sep 27, 2021 •

edited

Loading

piersy Sep 27, 2021 •

edited

Loading