Withdrawal intent 32byte pubkey #288

sapinb · 2024-09-16T14:15:10Z

Description

Update withdrawal intent to use 32bit pubkey
Burn value sent to withdrawal precompile
add additional checks in functional tests

@delbonis please reject this PR if you are already working on this.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Refactor

Checklist

I have performed a self-review of my code.
I have commented my code where necessary.
I have updated the documentation if needed.
My changes do not introduce new warnings.
I have added tests that prove my changes are effective or that my feature works.
New and existing tests pass with my changes.

Related Issues

burn bridge out amount add checks to funtional test

crates/reth/node/src/precompiles/bridge.rs

codecov · 2024-09-16T14:43:47Z

Codecov Report

Attention: Patch coverage is 2.04082% with 48 lines in your changes missing coverage. Please review.

Project coverage is 64.10%. Comparing base (d587d87) to head (aac8569).
Report is 31 commits behind head on master.

Files with missing lines	Patch %	Lines
crates/reth/node/src/precompiles/bridge.rs	0.00%	31 Missing ⚠️
crates/reth/node/src/utils.rs	0.00%	6 Missing ⚠️
crates/state/src/bridge_ops.rs	0.00%	6 Missing ⚠️
crates/evmexec/src/engine.rs	33.33%	2 Missing ⚠️
crates/reth/node/src/evm.rs	0.00%	2 Missing ⚠️
crates/reth/node/src/payload_builder.rs	0.00%	1 Missing ⚠️

@@            Coverage Diff             @@
##           master     #288      +/-   ##
==========================================
+ Coverage   58.73%   64.10%   +5.37%     
==========================================
  Files         168      184      +16     
  Lines       16340    19425    +3085     
==========================================
+ Hits         9597    12453    +2856     
- Misses       6743     6972     +229

Files with missing lines	Coverage Δ
crates/reth/node/src/primitives.rs	`0.00% <ø> (ø)`
crates/reth/node/src/payload_builder.rs	`0.00% <0.00%> (ø)`
crates/evmexec/src/engine.rs	`70.22% <33.33%> (ø)`
crates/reth/node/src/evm.rs	`0.00% <0.00%> (ø)`
crates/reth/node/src/utils.rs	`0.00% <0.00%> (ø)`
crates/state/src/bridge_ops.rs	`0.00% <0.00%> (ø)`
crates/reth/node/src/precompiles/bridge.rs	`0.00% <0.00%> (ø)`

... and 60 files with indirect coverage changes

Rajil1213

Left some nits. Otherwise, looks good. :)

crates/state/src/bridge_ops.rs

Rajil1213 · 2024-09-16T20:17:37Z

functional-tests/fn_el_bridge_precompile.py

-        # 1 eth
-        to_transfer = 1_000_000_000_000_000_000
+        # 10 rollup btc as wei
+        to_transfer_wei = 10_000_000_000_000_000_000


Should probably be added to constants.py so it's easier to change this in the future if necessary.

We should do that once we use this in other tests. Currently, this is the only test using these values.

crates/reth/node/src/precompiles/bridge.rs

crates/evmexec/src/engine.rs

delbonis · 2024-09-16T20:19:55Z

crates/reth/node/src/precompiles/bridge.rs

@@ -8,19 +8,19 @@ use crate::primitives::WithdrawalIntentEvent;

 // TODO: address?
 pub const BRIDGEOUT_ADDRESS: Address = address!("000000000000000000000000000000000b121d9e");
-const MIN_WITHDRAWAL_WEI: u128 = 1_000_000_000_000_000_000u128;
+const WITHDRAWAL_WEI: u128 = 10 * (1e18 as u128);


Should be set from params, we shouldn't assume 10 BTC is hardcoded.

This is in the reth binary and we haven't setup params for this yet. Will use constants for the time being.

crates/reth/node/src/precompiles/bridge.rs

delbonis · 2024-09-16T20:25:41Z

crates/reth/node/src/precompiles/bridge.rs

+            });
+        }
+
+        account.info.balance = new_balance;


Don't we have to write the account balance back to the state after making changes like this? Should have test(s) that checks to see if the account's balance actually decreases when they bridge out.

This code is to burn the bridge out amount transferred to the bridge contract. The changes on the sender's side is done by evm itself.

So shouldn't we just unconditionally set it to 0 then? Instead of doing the checked_sub that we expect will always go to zero? We can add a sanity check to it to ensure that it's doing it right.

delbonis · 2024-09-16T20:26:28Z

crates/reth/node/src/primitives.rs

+        /// 32 bytes pubkey for withdrawal address in L1
        bytes dest_pk,


bytes32

Also, I know this was implemented in the other ticket but is this really the best way to implement this? I thought StatefulPrecompile would have been more correct.

Was originally planning to do this with StatefulPrecompile, but ran into issues during implementation.
Specifically, StatefulPrecompile does not provide access to modify evm state, nor to send event logs. So there was no way to communicate the precompile call info easily to the payload builder.

Checking tx in the block manually in the builder would not work if the bridge out is called through a contract.

It might be possible to pass a shared state from the builder to the stateful precompile somehow, but that implementation was getting quite ugly.

So I used the context precompile to send event logs and handled is similar to how the beacon chain deposits were already being handled.

It might be possible to pass a shared state from the builder to the stateful precompile somehow, but that implementation was getting quite ugly.

Yeah I mean you'd have to use RefCell probably which is kinda messy but I believe it would work.

Regardless I suppose this is a workable solution. It does require special filtering which feels like it violates layers but it's hard to get around that.

crates/state/src/bridge_ops.rs

delbonis

Ahhh several review comments somehow didn't get picked up as review comments, I'm not sure if I screwed up or what. Make sure to read above for parts that didn't get included in this.

functional-tests/fn_el_bridge_precompile.py

delbonis · 2024-09-16T20:30:12Z

functional-tests/fn_el_bridge_precompile.py

+        assert event_data.args.dest_pk.hex() == dest_pk
+
+        final_block_no = web3.eth.block_number
+        final_bridge_balance = web3.eth.get_balance(dest)


Precompiles can't have balances do they? At least we shouldn't be modelling it like that because that screws up the accounting. Unless it's hard to do anything about it, in which case nevermind I guess this works out.

The precompile balance will always be zero as the value transferred to it is burned by the precompile.
There is an assertion below this that the final bridge balance should be zero.

delbonis · 2024-09-16T20:32:27Z

Also bytes not bits.

delbonis

Looks good, I understand the strategy with this now. I want to make sure that we're totally sure we're doing this in a sensible way so some of these comments might be unnecessary. Also some housekeeping.

delbonis · 2024-09-18T05:21:46Z

crates/reth/node/src/precompiles/bridge.rs

+/// The fixed withdrawal amount in wei (10 BTC equivalent).
+const FIXED_WITHDRAWAL_WEI: U256 = u256_from(10 * WEI_PER_BTC);


Is there a feasible path to making this configurable? Maybe make it a field for the precompile? Since we know we will support multiple withdrawal quantities. It would still end up being fixed, but there's no reason to hardcode it this low down in the impl.

delbonis · 2024-09-18T05:23:59Z

functional-tests/fn_el_bridge_precompile.py

The other thing is these defs might make more sense to be in constants.py or even just a json file in a new contrib toplevel dir.

delbonis · 2024-09-18T05:29:23Z

crates/reth/node/src/precompiles/bridge.rs

        evmctx.journaled_state.log(Log {
            address: BRIDGEOUT_ADDRESS,
            data: logdata,
        });

-        // burn value sent to bridge
-        let Ok((account, _)) = evmctx.load_account(BRIDGEOUT_ADDRESS) else {
-            // should never happen
-            return Err(PrecompileErrors::Fatal {
-                msg: "could not load account".into(),
-            });
-        };
-
-        let (new_balance, overflow) = account.info.balance.overflowing_sub(value);
-        if overflow {
-            // should never happen
-            return Err(PrecompileErrors::Fatal {
-                msg: "invalid balance".into(),
-            });
-        }
+        // Burn value sent to bridge by adjusting the account balance of bridge precompile
+        let (account, _) = evmctx
+            .load_account(BRIDGEOUT_ADDRESS)


This too, these don't need to be hardcoded, it's fine to read them from a field.

delbonis · 2024-09-18T05:30:45Z

crates/reth/node/src/precompiles/bridge.rs

+            .ok_or_else(|| PrecompileErrors::Fatal {
+                msg: "Insufficient balance in BRIDGEOUT_ADDRESS account".into(),
+            })?;


Should this revert the whole tx execution? If my understanding is correct this would be an exceptional case (as the comment mentions). See also comment about unconditionally setting to 0 anyways.

updated. setting it directly to zero

delbonis · 2024-09-18T05:31:14Z

crates/reth/node/src/precompiles/bridge.rs

+            });
+        }
+
+        account.info.balance = new_balance;


So shouldn't we just unconditionally set it to 0 then? Instead of doing the checked_sub that we expect will always go to zero? We can add a sanity check to it to ensure that it's doing it right.

delbonis · 2024-09-18T05:31:47Z

crates/reth/node/src/evm.rs

                crate::precompiles::bridge::BRIDGEOUT_ADDRESS,
                ContextPrecompile::ContextStateful(Arc::new(
-                    crate::precompiles::bridge::BridgeoutPrecompile::default(),
+                    crate::precompiles::bridge::BridgeoutPrecompile,


Is this fully-qualified crate:: import conventional?

sapinb added 2 commits September 16, 2024 13:25

use 32 byte calldata for bridge out precompile and withdrawal intent

686602b

require withdrawal to equal 10btc

464b765

burn bridge out amount add checks to funtional test

sapinb requested review from Rajil1213 and delbonis September 16, 2024 14:15

storopoli reviewed Sep 16, 2024

View reviewed changes

crates/reth/node/src/precompiles/bridge.rs Outdated Show resolved Hide resolved

Rajil1213 requested changes Sep 16, 2024

View reviewed changes

delbonis reviewed Sep 16, 2024

View reviewed changes

delbonis requested changes Sep 16, 2024

View reviewed changes

sapinb changed the title ~~Withdrawal intent 32bit pubkey~~ Withdrawal intent 32byte pubkey Sep 16, 2024

resolve pr comments

abf51dd

sapinb requested review from Rajil1213 and delbonis September 17, 2024 07:33

sapinb marked this pull request as ready for review September 17, 2024 07:33

delbonis requested changes Sep 18, 2024

View reviewed changes

address PR comments

aac8569

sapinb requested a review from delbonis September 18, 2024 16:40

delbonis approved these changes Sep 18, 2024

View reviewed changes

delbonis merged commit c22501f into master Sep 18, 2024
15 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Withdrawal intent 32byte pubkey #288

Withdrawal intent 32byte pubkey #288

sapinb commented Sep 16, 2024 •

edited

Loading

codecov bot commented Sep 16, 2024 •

edited

Loading

Rajil1213 left a comment

Rajil1213 Sep 16, 2024

sapinb Sep 17, 2024

delbonis Sep 16, 2024

sapinb Sep 17, 2024

delbonis Sep 16, 2024

sapinb Sep 16, 2024

delbonis Sep 18, 2024

sapinb Sep 18, 2024

delbonis Sep 16, 2024

sapinb Sep 16, 2024

delbonis Sep 18, 2024 •

edited

Loading

delbonis left a comment

delbonis Sep 16, 2024

sapinb Sep 17, 2024

delbonis commented Sep 16, 2024

delbonis left a comment

delbonis Sep 18, 2024

delbonis Sep 18, 2024

delbonis Sep 18, 2024

delbonis Sep 18, 2024

sapinb Sep 18, 2024

delbonis Sep 18, 2024

delbonis Sep 18, 2024

sapinb Sep 18, 2024

		/// 32 bytes pubkey for withdrawal address in L1
		bytes dest_pk,

		/// The fixed withdrawal amount in wei (10 BTC equivalent).
		const FIXED_WITHDRAWAL_WEI: U256 = u256_from(10 * WEI_PER_BTC);

Withdrawal intent 32byte pubkey #288

Withdrawal intent 32byte pubkey #288

Conversation

sapinb commented Sep 16, 2024 • edited Loading

Description

Type of Change

Checklist

Related Issues

codecov bot commented Sep 16, 2024 • edited Loading

Codecov Report

Rajil1213 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delbonis Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

delbonis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delbonis commented Sep 16, 2024

delbonis 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

sapinb commented Sep 16, 2024 •

edited

Loading

codecov bot commented Sep 16, 2024 •

edited

Loading

delbonis Sep 18, 2024 •

edited

Loading