Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(node): validator gas exemption #1295

Open
wants to merge 7 commits into
base: read-only-txn
Choose a base branch
from

Conversation

cryptoAtwill
Copy link
Contributor

@cryptoAtwill cryptoAtwill commented Feb 20, 2025

This PR introduces bottom up signature submission to the chain where txn gas will not be charged for validators.

Closes #1255


This change is Reviewable

@cryptoAtwill cryptoAtwill requested a review from a team as a code owner February 20, 2025 09:47
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 11 files reviewed, 4 unresolved discussions


fendermint/rpc/src/message.rs line 194 at r1 (raw file):

    }

    /// Send a validator message to the fendermint

nit: to fendermint


fendermint/vm/interpreter/src/chain.rs line 424 at r1 (raw file):

                }
            },
            ChainMessage::Validator(v) => match v {

JFYI: I don't have enough context yet to judge if this design is good or not


fendermint/vm/interpreter/src/validator.rs line 19 at r1 (raw file):

const SOLIDITY_SELECTOR_BYTES: usize = 8;

pub(crate) fn execute_bottom_up_signature<DB: Blockstore + Clone + 'static>(

A doc comment 'd be nice


fendermint/rpc/src/tx.rs line 115 at r1 (raw file):

        if calldata.len() < SOLIDITY_SELECTOR_BYTES {
            return Err(anyhow!("invalid validator calldata"));

nit: bail!()
nit: the error message is not actionable, nor does it yield enough info to debug, here we clearly check for length, so something like: received message is {x} bytes, but must be at least {SOLIDITY_SELECTOR_BYTES} for the selector

Copy link
Contributor Author

@cryptoAtwill cryptoAtwill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 12 files reviewed, 4 unresolved discussions (waiting on @drahnr)


fendermint/rpc/src/message.rs line 194 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

nit: to fendermint

updated!


fendermint/rpc/src/tx.rs line 115 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

nit: bail!()
nit: the error message is not actionable, nor does it yield enough info to debug, here we clearly check for length, so something like: received message is {x} bytes, but must be at least {SOLIDITY_SELECTOR_BYTES} for the selector

updated!


fendermint/vm/interpreter/src/chain.rs line 424 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

JFYI: I don't have enough context yet to judge if this design is good or not

take your time, can discuss over this again next monday as well.


fendermint/vm/interpreter/src/validator.rs line 19 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

A doc comment 'd be nice

added!

@cryptoAtwill cryptoAtwill requested a review from drahnr February 21, 2025 09:06
Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 13 files reviewed, 7 unresolved discussions


fendermint/rpc/src/tx.rs line 116 at r3 (raw file):

        if calldata.len() < SOLIDITY_SELECTOR_BYTES {
            return Err(anyhow!(
                "invalid validator calldata, expected at least {} but found only {}",

.. bytes


Cargo.toml line 250 at r3 (raw file):

[patch.crates-io]
fvm = { git = "https://github.com/consensus-shipyard/ref-fvm.git", branch = "customize-gas" }

Are we planning to merge that branch into our forked main?


fendermint/rpc/src/message.rs line 208 at r3 (raw file):

            .inner
            .transaction(to, method_num, params, value, gas_params);
        let signed = SignedMessage::new_secp256k1(message, &self.sk, &self.chain_id)?;

Can be future refactor, but I really would like to see the following pattern:

let signed_msg: Signed<Message> = Message::new().sign(keystore, pubk, chain_id);
// and reverse
let signed_msg: Signed<Message> = bytes.parse()?;
let message: Message = signed_msg.verify()?;

fendermint/testing/materializer/tests/docker_tests/layer2.rs line 22 at r3 (raw file):

const CHECKPOINT_PERIOD: u64 = 2;
const SLEEP_SECS: u64 = 5;
const MAX_RETRIES: u32 = 10;

Can we leave a comment why we need so many retries and such a short checkpoint period?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants