remove proptest specific encoding logic #6503

supernovahs · 2024-02-09T12:40:07Z

supernovahs · 2024-02-09T12:40:40Z

Getting compiler error

error: local ambiguity when calling macro `prop_compose`: multiple parsing options: built-in NTs tt ('arg') or 1 other option.
   --> crates/primitives/src/header.rs:947:49
    |
947 |                 eip_4844_active in any::<bool>(),

Any idea why ?

Rjected · 2024-02-09T15:48:07Z

Getting compiler error

error: local ambiguity when calling macro `prop_compose`: multiple parsing options: built-in NTs tt ('arg') or 1 other option.
   --> crates/primitives/src/header.rs:947:49
    |
947 |                 eip_4844_active in any::<bool>(),

Any idea why ?

This looks like an internal proptest macro thing, I'd start by either changing how compose is being used, check syntax with the proptest_compose source code, or if none of that works, just use something other than proptest_compose.

supernovahs · 2024-02-11T11:51:14Z

I think I have to change this value for the tests to pass

reth/crates/storage/provider/src/test_utils/blocks.rs

Line 56 in 8cfa5ef

    
           const BLOCK_RLP: [u8; 610] = hex!("f9025ff901f7a0c86e8cc0310ae7c531c758678ddbfd16fc51c8cef8cec650b032de9869e8b94fa01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347942adc25665018aa1fe0e6bc666dac8fc2697ff9baa050554882fbbda2c2fd93fdc466db9946ea262a67f7a76cc169e714f105ab583da00967f09ef1dfed20c0eacfaa94d5cd4002eda3242ac47eae68972d07b106d192a0e3c8b47fbfc94667ef4cceb17e5cc21e3b1eebd442cebb27f07562b33836290db90100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008302000001830f42408238108203e800a00000000000000000000000000000000000000000000000000000000000000000880000000000000000f862f860800a83061a8094095e7baea6a6c7c4c2dfeb977efac326af552d8780801ba072ed817487b84ba367d15d2f039b5fc5f087d0a8882fbdf73e8cb49357e1ce30a0403d800545b8fc544f92ce8124e2255f8c3c6af93f28243a120585d4c4c6a2a3c0");

Rjected · 2024-02-13T23:01:27Z

I think I have to change this value for the tests to pass

reth/crates/storage/provider/src/test_utils/blocks.rs

Line 56 in 8cfa5ef

const BLOCK_RLP: [u8; 610] = hex!("f9025ff901f7a0c86e8cc0310ae7c531c758678ddbfd16fc51c8cef8cec650b032de9869e8b94fa01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347942adc25665018aa1fe0e6bc666dac8fc2697ff9baa050554882fbbda2c2fd93fdc466db9946ea262a67f7a76cc169e714f105ab583da00967f09ef1dfed20c0eacfaa94d5cd4002eda3242ac47eae68972d07b106d192a0e3c8b47fbfc94667ef4cceb17e5cc21e3b1eebd442cebb27f07562b33836290db90100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008302000001830f42408238108203e800a00000000000000000000000000000000000000000000000000000000000000000880000000000000000f862f860800a83061a8094095e7baea6a6c7c4c2dfeb977efac326af552d8780801ba072ed817487b84ba367d15d2f039b5fc5f087d0a8882fbdf73e8cb49357e1ce30a0403d800545b8fc544f92ce8124e2255f8c3c6af93f28243a120585d4c4c6a2a3c0");

no, that should not be changed unless it contains some of the "fake" values from the previous encoding impl. you'd have to manually check that the rlp string is correct

Rjected

some questions about derive_arbitrary impls, also looks like lints need to be fixed

crates/net/eth-wire/src/types/broadcast.rs

Rjected · 2024-02-13T23:05:50Z

crates/primitives/src/header.rs

+        // Set fields based on EIP-1559 activation
+        if header.base_fee_per_gas.is_none() {
+            // EIP-1559 not active, clear related fields
+            header.withdrawals_root = None;
+            header.blob_gas_used = None;
+            header.excess_blob_gas = None;
+            header.parent_beacon_block_root = None;
+        } else {
+            // 4895 is not active
+            if header.withdrawals_root.is_none() {
+                header.blob_gas_used = None;
+                header.excess_blob_gas = None;
+                header.parent_beacon_block_root = None;
+            } else {
+                // 4895 is active
+                if eip_4844_active {
+                    // EIP-4844 is active
+                    header.blob_gas_used = Some(blob_gas_used);
+                    header.excess_blob_gas = Some(excess_blob_gas);
+                    header.parent_beacon_block_root = Some(parent_beacon_block_root);
+                } else {
+                    // EIP-4844 is not active
+                    header.blob_gas_used = None;
+                    header.excess_blob_gas = None;
+                    header.parent_beacon_block_root = None;
+                };


I would try to flatten this if possible

crates/primitives/src/block.rs

supernovahs · 2024-02-14T04:03:51Z

I think I have to change this value for the tests to pass

reth/crates/storage/provider/src/test_utils/blocks.rs

Line 56 in 8cfa5ef

const BLOCK_RLP: [u8; 610] = hex!("f9025ff901f7a0c86e8cc0310ae7c531c758678ddbfd16fc51c8cef8cec650b032de9869e8b94fa01dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347942adc25665018aa1fe0e6bc666dac8fc2697ff9baa050554882fbbda2c2fd93fdc466db9946ea262a67f7a76cc169e714f105ab583da00967f09ef1dfed20c0eacfaa94d5cd4002eda3242ac47eae68972d07b106d192a0e3c8b47fbfc94667ef4cceb17e5cc21e3b1eebd442cebb27f07562b33836290db90100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000008302000001830f42408238108203e800a00000000000000000000000000000000000000000000000000000000000000000880000000000000000f862f860800a83061a8094095e7baea6a6c7c4c2dfeb977efac326af552d8780801ba072ed817487b84ba367d15d2f039b5fc5f087d0a8882fbdf73e8cb49357e1ce30a0403d800545b8fc544f92ce8124e2255f8c3c6af93f28243a120585d4c4c6a2a3c0");

no, that should not be changed unless it contains some of the "fake" values from the previous encoding impl. you'd have to manually check that the rlp string is correct

yeah this encoding turned out to be correct after I checked manually

supernovahs · 2024-02-14T06:22:26Z

lololol
I implemented manual impls for SealedHeader, not Header 🤣

Rjected · 2024-02-16T21:09:26Z

hey @supernovahs any updates here?

supernovahs · 2024-02-17T04:15:28Z

@Rjected Are you ok with the current way of implementing manual impls for SealedHeader or do you want the Header Aribtrary impls to be implemented manually instead?
The latter also have some complexity of since it involves removing main_codec from many structs .

Rjected · 2024-02-21T20:14:23Z

@Rjected Are you ok with the current way of implementing manual impls for SealedHeader or do you want the Header Aribtrary impls to be implemented manually instead? The latter also have some complexity of since it involves removing main_codec from many structs .

@supernovahs we should not remove main_codec, so implementing on sealedheader here is fine

supernovahs · 2024-02-23T06:25:22Z

Looking

supernovahs · 2024-02-23T12:29:37Z

ci issue i think

Rjected

a few comments and style nits, great progress!

crates/primitives/src/block.rs

crates/primitives/src/header.rs

crates/net/eth-wire/src/types/blocks.rs

crates/primitives/src/block.rs

crates/primitives/src/header.rs

crates/primitives/src/block.rs

crates/net/eth-wire/src/types/blocks.rs

crates/primitives/src/block.rs

onbjerg · 2024-02-26T14:11:43Z

few nits, looks great :)

mattsse

@Rjected please wrap up here

Rjected

did a lot of cleanup, @onbjerg bump for review

completed

Co-authored-by: Dan Cline <6798349+Rjected@users.noreply.github.com>

Rjected requested changes Feb 13, 2024

View reviewed changes

emhane added C-enhancement New feature or request C-test A change that impacts how or what we test labels Feb 17, 2024

supernovahs marked this pull request as ready for review February 23, 2024 09:47

supernovahs requested review from rakita, joshieDo, onbjerg, rkrasiuk, emhane, mattsse and gakonst as code owners February 23, 2024 09:47

mattsse requested a review from Rjected February 23, 2024 17:04

Rjected requested changes Feb 23, 2024

View reviewed changes

onbjerg previously requested changes Feb 26, 2024

View reviewed changes

crates/net/eth-wire/src/types/blocks.rs Outdated Show resolved Hide resolved

crates/primitives/src/block.rs Outdated Show resolved Hide resolved

mattsse approved these changes Feb 27, 2024

View reviewed changes

Rjected force-pushed the proptest_logic_header branch from 9d59e80 to 0a81006 Compare February 28, 2024 05:08

supernovahs requested a review from shekhirin as a code owner February 28, 2024 05:08

update

cc9a5bd

supernovahs and others added 16 commits February 28, 2024 00:43

nit

9ab5be4

update

2b5e8d6

use manual arbitrary impls at various structs which use Header

a183703

nits

15b6702

clippy

e14b87a

use custom header test in SealedBlock::arbitrary also

6bef07d

flatten if else

60bed53

clip

6ccf5da

collapsable if else

67293c1

clippy

f73d48b

nits

003ba7e

add_arbitrary_tests rlp add

c6b582a

use valid header strategy only

98c11ab

missed a collapsible if to be changed

2d85dd9

nits

0f4f2cd

chore: fix cargo hack check

4d5dda7

Rjected force-pushed the proptest_logic_header branch from 0a81006 to 4d5dda7 Compare February 28, 2024 05:44

Rjected requested a review from onbjerg February 28, 2024 05:48

Rjected added 4 commits February 28, 2024 13:04

cleanup and derive more

d9087cc

reusable strategy and generation method

868b704

feature gate imports

aaeb562

format

1a4c544

Rjected self-requested a review February 28, 2024 21:32

Rjected approved these changes Feb 28, 2024

View reviewed changes

mattsse enabled auto-merge February 28, 2024 21:45

mattsse added this pull request to the merge queue Feb 28, 2024

Merged via the queue into paradigmxyz:main with commit 3519270 Feb 28, 2024
29 checks passed

fgimenez pushed a commit to fgimenez/reth that referenced this pull request Feb 29, 2024

remove proptest specific encoding logic (paradigmxyz#6503)

d0784ef

Co-authored-by: Dan Cline <6798349+Rjected@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove proptest specific encoding logic #6503

remove proptest specific encoding logic #6503

remove proptest specific encoding logic #6503

remove proptest specific encoding logic #6503

Conversation

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