Enforce base fee #1390

jbearer · 2024-04-26T20:01:04Z

Add logic in validate_proposal to enforce that the provided fee is at least the base fee
Enable base fees in the demo, enforcing in the sequencers and configuring the builder to pay the base fee
Deploy bridge contract before starting sequencers, read deposits from here
Add a tool to work with the bridge, use this to fund the builder instead of prefunded accounts

The new demo configuration should work on a real network like Sepolia or Ethereum, and demonstrates the startup order/dependencies.

Draft because I think this should go in after the latest builder and HotShot updates, but I don't know how to do those updates.

Testing

A good end-to-end test is to run demo-native, convince yourself that it's working as you normally would (look at block height, submit-transactions logs, etc) and then watch the balance of the builder account and burner account:

cargo run --release --all-features --bin bridge -- balance -e http://localhost:44001 -a 0x23618e81E3f5cdF7f54C3d65f7FBc0aBf5B21E8f
cargo run --release --all-features --bin bridge -- balance -e http://localhost:44001 -a 0x0000000000000000000000000000000000000000

The former should be steadily decreasing while the latter is steadily increasing, as the builder pays fees

move47

/builder changes looks good.

move47 · 2024-04-29T01:00:25Z

I think this can go first and later we can update with hotshot 0.5.45(needs to release tag) which will have simplify builder interaction, and update builder along with that.

jbearer · 2024-04-29T15:19:27Z

We need to wait until @Ancient123 is ready for the new contracts deployment order

docker-compose.yaml

sequencer/src/bin/bridge.rs

sveitser · 2024-05-14T12:43:16Z

scripts/smoke-test-demo

+function get_balance() {
+	(
+		unset MNEMONIC
+		cargo run --release --all-features --bin bridge -- balance -e $SEQUENCER_API -a $1 -b $2


Not a huge deal but I think it's kind of nice that this script is currently portable and doesn't need a rust toolchain. Maybe it would be useful to expose reading the bridge query APIs via the explorer then we could query that instead. If we do add the cargo run here we should probably add rust-cache to the test-demo github action workflow.

What do you mean exactly by reading the bridge APIs via the explorer?

Another approach could be to use bridge as an executable for the native demo (since the native demo already relies on cargo targets being added to $PATH) and use the bridge docker for the docker demo, which would also have the nice benefit of testing that the new docker is built correctly. I could add a CLI flag to this script to choose the bridge executable

I mean if we had a HTTP API that allowed to query the bridge balances either run by the hotshot node or via the block explorer that would be useful here and I think it would be convenient with anyone interacting with our sequencer too but it's also somewhat out of scope.

We could also call bridge if it's in the PATH and otherwise try the docker container then we don't need a flag.

I mean if we had a HTTP API that allowed to query the bridge balances either run by the hotshot node or via the block explorer that would be useful here and I think it would be convenient with anyone interacting with our sequencer too but it's also somewhat out of scope.

I agree, but yeah, I think adding this to the block explorer is a lot of work. We do have an HTTP API (that is how the bridge tool gets its information) but it outputs a merkle path and the balance is base64 encoded which makes it a bit annoying to work with, hence the bridge tool.

sequencer/src/chain_config.rs

The collection of fees (or lack thereof) is not completely controlled by ChainConfig. This makes it possible to launch without fees (even without a fee contract) and upgrade to enable fees at a later date. The only code change required to do this upgraded will be the ability to change the ChainConfig by upgrading. After that, only a configuration change is need to change the ChainConfig to set the fee contract and base fee.

This will allow the underlying asset to be conserved, and we can decide what to do with the burnt fees at a later point.

* Use fee message defined in HotShot trait, rather than constructing it ourselves, to avoid accidentally going out of sync with how the builder is signing * Fail signature recovery if the recovered address is wrong, rather than proceeding with a random address

tbro · 2024-05-14T17:35:21Z

sequencer/src/state.rs

-            Err(err)
-        } else {
-            Ok(())
+            return Err(err);


Not really an issue w/ this PR, but I wonder if persistent_update_with should take a closure that returns a Result instead of an Option. I think we could then bubble the error up to the caller instead of mutating err. This code appears to demonstrate that we do need to handle more than just the Some and None cases. I might be confused though.

Yeah I think that would be cleaner. This pattern is basically just implementing that in a messier way. @mrain maybe you can clean this up in jf-merkle-tree whenever you find yourself with some spare time?

sequencer/src/l1_client.rs

sequencer/src/state.rs

sveitser · 2024-05-15T10:15:03Z

sequencer/src/state.rs

+        let header = parent.get_block_header();
+
+        // Validation fails because the genesis fee (0) is too low.
+        let err = validate_proposal(&state, instance.chain_config, &parent, header, &vid_common)


Inspect the error to ensure that it failed because of the base fee being too low?

We will do this in the followup #1463

sequencer/src/state.rs

sveitser · 2024-05-15T10:26:29Z

sequencer/src/state.rs

    anyhow::ensure!(
-        (VidSchemeType::get_payload_byte_len(vid_common) as u64)
-            < expected_chain_config.max_block_size(),
+        block_size < expected_chain_config.max_block_size,


This was added some time ago but wouldn't it be more intuitive to make the range inclusive so that a block of max_block_size is still allowed? If yes, let's add an issue.

sveitser · 2024-05-15T10:31:25Z

sequencer/src/state.rs

@@ -90,6 +94,14 @@ impl ValidatedState {
        self.fee_merkle_tree.update(account, amount).unwrap();
    }

+    pub fn balance(&mut self, account: FeeAccount) -> Option<FeeAmount> {


This function is only used in tests (unless my rust-analyzer is tripping). I'm not sure returning None is a good idea for the NotInMemory case, but if we make it test code only then I think it's not a problem.

I know we don't have a stable public Rust API yet but I feel like a function like this should be part of it. Could easily be useful outside tests for clients and apps built on top of this. What do you think such a public function should do in the NotInMemory case? Should it return Result instead of Option?

sveitser

The request for change is because I think we should avoid the term "burn" unless we do light things on fire.

The other comments are just suggestions / nitpicky stuff.

Use the bridge executable from the path if it is already built; otherwise use Docker.

jbearer requested review from Ancient123 and move47 April 26, 2024 20:01

move47 reviewed Apr 26, 2024

View reviewed changes

sveitser reviewed Apr 30, 2024

View reviewed changes

docker-compose.yaml Show resolved Hide resolved

sveitser reviewed Apr 30, 2024

View reviewed changes

sequencer/src/bin/bridge.rs Show resolved Hide resolved

jbearer force-pushed the jb/enforce-base-fee branch 6 times, most recently from 9b197de to a82f075 Compare May 7, 2024 19:18

jbearer marked this pull request as ready for review May 7, 2024 19:31

jbearer requested review from nomaxg, philippecamacho, ImJeremyHe and tbro as code owners May 7, 2024 19:31

jbearer force-pushed the jb/enforce-base-fee branch 3 times, most recently from a46cf22 to 21bd047 Compare May 10, 2024 13:30

jbearer mentioned this pull request May 10, 2024

Define a genesis config file #1443

Merged

jbearer force-pushed the jb/enforce-base-fee branch from 21bd047 to bbb4064 Compare May 12, 2024 19:16

tbro reviewed May 13, 2024

View reviewed changes

sequencer/src/bin/bridge.rs Outdated Show resolved Hide resolved

sveitser reviewed May 14, 2024

View reviewed changes

sequencer/src/chain_config.rs Outdated Show resolved Hide resolved

jbearer added 4 commits May 14, 2024 09:27

Enforce base fee

d160009

Update builder to pay the appropriate base fee

790181e

Add a tool for using the bridge

10c7d59

Build bridge tool docker

44bdb72

jbearer added 6 commits May 14, 2024 09:28

Save burned fees in a burner account

4913acb

This will allow the underlying asset to be conserved, and we can decide what to do with the burnt fees at a later point.

Add balance checks to demo smoketest

75ba40c

Fix merge mistake

b82cbc6

Add comments

2ac26c0

jbearer force-pushed the jb/enforce-base-fee branch from bbb4064 to 2ac26c0 Compare May 14, 2024 13:31

tbro reviewed May 14, 2024

View reviewed changes

sveitser reviewed May 15, 2024

View reviewed changes

sequencer/src/l1_client.rs Show resolved Hide resolved

sveitser reviewed May 15, 2024

View reviewed changes

sequencer/src/state.rs Show resolved Hide resolved

sveitser reviewed May 15, 2024

View reviewed changes

sequencer/src/state.rs Outdated Show resolved Hide resolved

sveitser reviewed May 15, 2024

View reviewed changes

sveitser requested changes May 15, 2024

View reviewed changes

jbearer added 2 commits May 15, 2024 08:58

Rename burn account to recipient

4d34e25

Don't depend on cargo in smoke-test-demo

61c3086

Use the bridge executable from the path if it is already built; otherwise use Docker.

sveitser self-requested a review May 15, 2024 14:21

sveitser approved these changes May 15, 2024

View reviewed changes

jbearer merged commit 97548f0 into main May 15, 2024
15 checks passed

jbearer deleted the jb/enforce-base-fee branch May 15, 2024 15:39

sveitser mentioned this pull request May 28, 2024

CI: docker demo is broken #1508

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce base fee #1390

Enforce base fee #1390

jbearer commented Apr 26, 2024 •

edited

Loading

move47 left a comment

move47 commented Apr 29, 2024

jbearer commented Apr 29, 2024

sveitser May 14, 2024 •

edited

Loading

jbearer May 14, 2024

sveitser May 15, 2024

jbearer May 15, 2024

jbearer May 15, 2024

tbro May 14, 2024 •

edited

Loading

jbearer May 14, 2024

sveitser May 15, 2024

jbearer May 15, 2024

sveitser May 15, 2024

jbearer May 15, 2024

sveitser May 15, 2024

jbearer May 15, 2024

sveitser left a comment

Enforce base fee #1390

Enforce base fee #1390

Conversation

jbearer commented Apr 26, 2024 • edited Loading

Testing

move47 left a comment

Choose a reason for hiding this comment

move47 commented Apr 29, 2024

jbearer commented Apr 29, 2024

sveitser May 14, 2024 • edited Loading

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

tbro May 14, 2024 • edited Loading

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

sveitser left a comment

Choose a reason for hiding this comment

jbearer commented Apr 26, 2024 •

edited

Loading

sveitser May 14, 2024 •

edited

Loading

tbro May 14, 2024 •

edited

Loading