Skip to content

Commit

Permalink
Fail validation, but don't panic, if builder fee exceeds u64::MAX
Browse files Browse the repository at this point in the history 
Since the builder fee is actually being charged now, instead of silently
ignored, I had to update all the tests to prefund the builder account. I
deleted `test_catchup` because it wasn't working nicely with prefunding
the builder account with a forgotten state, but it wasn't testing anything
that the newer `test_catchup_live` wasn't also testing in a better way.
  • Loading branch information
@jbearer
jbearer committed Apr 25, 2024
1 parent d7d7431 commit 0c64fbf
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 117 deletions.
2 changes: 1 addition & 1 deletion Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

107 changes: 6 additions & 101 deletions sequencer/src/api.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -750,15 +750,14 @@ mod test {
use super::*;
use crate::{
catchup::{mock::MockStateCatchup, StatePeers},
empty_builder_commitment,
persistence::no_storage::NoStorage,
state::{FeeAccount, FeeAmount},
testing::TestConfig,
Header, NodeState,
Header,
};
use async_compatibility_layer::logging::{setup_backtrace, setup_logging};
use async_std::task::sleep;
use committable::{Commitment, Committable};
use committable::Commitment;
use es_version::{SequencerVersion, SEQUENCER_VERSION};
use futures::{
future::{self, join_all},
Expand All @@ -771,14 +770,9 @@ mod test {
};
use hotshot_types::{
event::LeafInfo,
traits::{
block_contents::BlockHeader, metrics::NoMetrics, node_implementation::ConsensusTime,
},
};
use jf_primitives::merkle_tree::{
prelude::{MerkleProof, Sha3Node},
AppendableMerkleTreeScheme,
traits::{metrics::NoMetrics, node_implementation::ConsensusTime},
};
use jf_primitives::merkle_tree::prelude::{MerkleProof, Sha3Node};
use portpicker::pick_unused_port;
use std::time::Duration;
use surf_disco::Client;
Expand Down Expand Up @@ -839,18 +833,7 @@ mod test {
.status(Default::default()),
);

// Populate the builder account so we have something to look up later.
let account = TestConfig::builder_key().fee_account();
let mut state = ValidatedState::default();
state.prefund_account(account, 1.into());
let mut network = TestNetwork::with_state(
options,
std::array::from_fn(|_| state.clone()),
[NoStorage; TestConfig::NUM_NODES],
std::array::from_fn(|_| MockStateCatchup::default()),
)
.await;

let mut network = TestNetwork::new(options, [NoStorage; TestConfig::NUM_NODES]).await;
let url = format!("http://localhost:{port}").parse().unwrap();
let client: Client<ServerError, SequencerVersion> = Client::new(url);

Expand Down Expand Up @@ -887,6 +870,7 @@ mod test {
assert_eq!(*path.elem().unwrap(), block.hash());

tracing::info!(i, "get fee state");
let account = TestConfig::builder_key().fee_account();
let path = client
.get::<MerkleProof<FeeAmount, FeeAccount, Sha3Node, 256>>(&format!(
"fee-state/{}/{}",
Expand All @@ -906,85 +890,6 @@ mod test {
setup_logging();
setup_backtrace();

// Create some non-trivial initial state. We will give all the nodes in the network this
// state, except for one, which will have a forgotten state and need to catch up.
let mut state = ValidatedState::default();
// Prefund an arbitrary account so the fee state has some data to forget.
state.prefund_account(Default::default(), 1000.into());
// Push an arbitrary header commitment so the block state has some data to forget.
state
.block_merkle_tree
.push(
Header::genesis(
&NodeState::mock(),
Default::default(),
empty_builder_commitment(),
Default::default(),
)
.commit(),
)
.unwrap();
let states = std::array::from_fn(|i| {
if i == TestConfig::NUM_NODES - 1 {
state.forget()
} else {
state.clone()
}
});

// Start a sequencer network, using the query service for catchup.
let port = pick_unused_port().expect("No ports free");
let network = TestNetwork::with_state(
Options::from(options::Http { port }).catchup(Default::default()),
states,
[NoStorage; TestConfig::NUM_NODES],
std::array::from_fn(|_| {
StatePeers::<SequencerVersion>::from_urls(vec![format!("http://localhost:{port}")
.parse()
.unwrap()])
}),
)
.await;
let mut events = network.server.get_event_stream();

// Wait for a (non-genesis) block proposed by each node, to prove that the lagging node has
// caught up and all nodes are in sync.
let mut proposers = [false; TestConfig::NUM_NODES];
loop {
let event = events.next().await.unwrap();
let EventType::Decide { leaf_chain, .. } = event.event else {
continue;
};
for LeafInfo { leaf, .. } in leaf_chain.iter().rev() {
let height = leaf.get_height();
let leaf_builder =
(leaf.get_view_number().get_u64() as usize) % TestConfig::NUM_NODES;
if height == 0 {
continue;
}

tracing::info!(
"waiting for blocks from {proposers:?}, block {height} is from {leaf_builder}",
);
proposers[leaf_builder] = true;
}

if proposers.iter().all(|has_proposed| *has_proposed) {
break;
}
}
}

#[async_std::test]
async fn test_catchup_live() {
setup_logging();
setup_backtrace();

// Similar to `test_catchup`, but instead of _starting_ with a non-trivial state and a node
// that is missing the state, we start consensus normally, wait for it to make some
// progress, stop one node and let it fall behind, then start it again and check that it
// catches up.

// Start a sequencer network, using the query service for catchup.
let port = pick_unused_port().expect("No ports free");
let mut network = TestNetwork::with_state(
Expand Down
22 changes: 13 additions & 9 deletions sequencer/src/header.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::{
block::{entry::TxTableEntryWord, tables::NameSpaceTable, NsTable},
chain_config::ResolvableChainConfig,
l1_client::L1Snapshot,
state::{BlockMerkleCommitment, FeeAccount, FeeAmount, FeeInfo, FeeMerkleCommitment},
state::{BlockMerkleCommitment, FeeAccount, FeeInfo, FeeMerkleCommitment},
ChainConfig, L1BlockInfo, Leaf, NodeState, SeqTypes, ValidatedState,
};
use anyhow::Context;
Expand Down Expand Up @@ -231,12 +231,15 @@ impl Header {
/// This message relates the fee info in this header to the payload corresponding to the header.
/// The message is signed by the builder (or whoever is paying for inclusion of the block) and
/// validated by consensus, as authentication for charging the fee to the builder account.
pub fn fee_message(&self) -> Vec<u8> {
fee_message(
self.fee_info.amount(),
pub fn fee_message(&self) -> anyhow::Result<Vec<u8>> {
Ok(fee_message(
self.fee_info.amount().as_u64().context(format!(
"fee amount out of range: {:?}",
self.fee_info.amount()
))?,
self.payload_commitment,
self.metadata(),
)
))
}
}

Expand All @@ -258,7 +261,7 @@ impl BlockHeader<SeqTypes> for Header {

// Validate the builder's signature, recovering their fee account address so that we can
// fetch the account state if missing.
let fee_msg = fee_message(builder_fee.fee_amount.into(), payload_commitment, &metadata);
let fee_msg = fee_message(builder_fee.fee_amount, payload_commitment, &metadata);
let builder_account = FeeAccount::from(
builder_fee
.fee_signature
Expand Down Expand Up @@ -340,7 +343,8 @@ impl BlockHeader<SeqTypes> for Header {
validated_state,
instance_state.chain_config,
)
.expect("invalid proposal")
// TODO we should be able to return an error from `Header::new`
.unwrap_or_else(|err| panic!("invalid proposal: {err:#}"))
}

fn genesis(
Expand Down Expand Up @@ -393,12 +397,12 @@ impl BlockHeader<SeqTypes> for Header {
}

fn fee_message(
amount: FeeAmount,
amount: u64,
payload_commitment: VidCommitment,
metadata: &<<SeqTypes as NodeType>::BlockPayload as BlockPayload>::Metadata,
) -> Vec<u8> {
let mut data = vec![];
data.extend_from_slice(amount.as_u64().to_be_bytes().as_ref());
data.extend_from_slice(amount.to_be_bytes().as_ref());
data.extend_from_slice(metadata.encode().as_ref());
data.extend_from_slice(payload_commitment.as_ref());
data
Expand Down
13 changes: 10 additions & 3 deletions sequencer/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -459,8 +459,11 @@ pub mod testing {
const STAKE_TABLE_CAPACITY_FOR_TEST: usize = 10;

pub async fn run_test_builder() -> (Option<Box<dyn BuilderTask<SeqTypes>>>, Url) {
<SimpleBuilderImplementation as TestBuilderImplementation<SeqTypes>>::start(1usize, ())
.await
<SimpleBuilderImplementation as TestBuilderImplementation<SeqTypes>>::start(
TestConfig::NUM_NODES,
(),
)
.await
}

#[derive(Clone)]
Expand Down Expand Up @@ -573,7 +576,7 @@ pub mod testing {
pub async fn init_node<Ver: StaticVersionType + 'static, P: SequencerPersistence>(
&self,
i: usize,
state: ValidatedState,
mut state: ValidatedState,
persistence: P,
catchup: impl StateCatchup + 'static,
metrics: &dyn Metrics,
Expand Down Expand Up @@ -603,6 +606,10 @@ pub mod testing {
_pd: Default::default(),
};

// Make sure the builder account is funded.
let builder_account = Self::builder_key().fee_account();
tracing::info!(%builder_account, "prefunding builder account");
state.prefund_account(builder_account, U256::max_value().into());
let node_state = NodeState::new(
ChainConfig::default(),
L1Client::new(self.anvil.endpoint().parse().unwrap(), Address::default()),
Expand Down
10 changes: 7 additions & 3 deletions sequencer/src/state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ fn validate_builder_fee(proposed_header: &Header) -> anyhow::Result<()> {
let signature = proposed_header
.builder_signature
.ok_or_else(|| anyhow::anyhow!("Builder signature not found"))?;
let msg = proposed_header.fee_message();
let msg = proposed_header.fee_message().context("invalid fee")?;
// verify signature
anyhow::ensure!(
proposed_header
Expand Down Expand Up @@ -965,8 +965,12 @@ impl CheckedSub for FeeAmount {
}

impl FeeAmount {
pub(crate) fn as_u64(&self) -> u64 {
self.0.as_u64()
pub(crate) fn as_u64(&self) -> Option<u64> {
if self.0 <= u64::MAX.into() {
Some(self.0.as_u64())
} else {
None
}
}
}

Expand Down

0 comments on commit 0c64fbf

Please sign in to comment.