Skip to content

Commit

Permalink
Fix uint64 underflow in SequencerClient (#2266)
Browse files Browse the repository at this point in the history
This underflow causes the fund-builder service to query for the block
with block number max(uint64) when the demo is started and the current
block height is still zero.

To test 

```
scripts/build-docker-images-native
just demo --force-recreate --renew-anon-volumes 
``` 
and run `scripts/smoke-test-demo` in another terminal.

Also increases the number of retries from 5 to 10, so we would retry for
a total of 2 seconds instead of 1. I figured it doesn't hurt to fail a
second later.

Adds a very basic regression test that panics with underflow on the old implementation.
  • Loading branch information
sveitser authored Nov 8, 2024
2 parents dfb2c77 + 558e2d4 commit 092ad42
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 13 deletions.
41 changes: 29 additions & 12 deletions client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,22 +63,21 @@ impl SequencerClient {
block: Option<u64>,
) -> anyhow::Result<FeeAmount> {
// Get the block height to query at, defaulting to the latest block.
let block = if let Some(block) = block {
block - 1
} else {
self.0
.get::<u64>("node/block-height")
.send()
.await
.context("getting block height")?
- 1
let mut block = match block {
Some(block) => block,
None => self.get_height().await?,
};

// As of block zero the state is empty, and the balance will be zero.
if block == 0 {
return Ok(0.into());
}
// Block is non-zero, we can safely decrement to query the state as of the previous block.
block -= 1;
// Download the Merkle path for this fee account at the specified block height. Transient errors
// are possible (for example, if we are fetching from the latest block, the block height might
// get incremented slightly before the state becomes available) so retry a few times.
// get incremented shortly before the state becomes available) so retry a few times.
let mut retry = 0;
let max_retries = 5;
let max_retries = 10;
let proof = loop {
tracing::debug!(%address, block, retry, "fetching Espresso balance");
match self
Expand Down Expand Up @@ -107,3 +106,21 @@ impl SequencerClient {
Ok(balance)
}
}

#[cfg(test)]
mod tests {
use super::*;
// Regression test for a bug where the block number underflowed. This test would panic
// on the previous implementation, as long as overflow checks are enabled.
#[async_std::test]
async fn test_regression_block_number_underflow() {
let client = SequencerClient::new("http://dummy-url:3030".parse().unwrap());
assert_eq!(
client
.get_espresso_balance(Address::zero(), Some(0))
.await
.unwrap(),
0.into()
)
}
}
2 changes: 2 additions & 0 deletions docker-compose.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ services:
- ACCOUNT_INDEX=$ESPRESSO_BUILDER_ETH_ACCOUNT_INDEX
- AMOUNT=1000000000000000000
- CONFIRMATIONS=1
- RUST_LOG
- RUST_LOG_FORMAT
depends_on:
deploy-sequencer-contracts:
condition: service_completed_successfully
Expand Down
2 changes: 1 addition & 1 deletion sequencer/src/bin/espresso-bridge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ async fn deposit(opt: Deposit) -> anyhow::Result<()> {
let initial_balance = espresso
.get_espresso_balance(l1.address(), None)
.await
.context("getting Espresso block height")?;
.context("getting Espresso balance")?;
tracing::debug!(%initial_balance, "initial balance");

// Send the deposit transaction.
Expand Down

0 comments on commit 092ad42

Please sign in to comment.