-
Notifications
You must be signed in to change notification settings - Fork 0
Timeboost Bundle Format #296
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
Conversation
c903423
to
68ae028
Compare
41ade11
to
4a5829a
Compare
4a5829a
to
eaf180a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting so much effort into getting the bundle format details right!
} | ||
|
||
#[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)] | ||
pub struct Bundle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this should be named RegularBundle
and BundleVariant
can become Bundle
?
timeboost-types/src/bundle.rs
Outdated
) -> arbitrary::Result<PriorityBundle<Signed>> { | ||
let bundle = Bundle::arbitrary(u)?; | ||
let auction = Address::default(); | ||
let seqno = SeqNo::from(u.int_in_range(1..=u64::MAX)?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the sequence number space is that large, chances are that there will always be gaps in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addendum: do we hit any tricky behavior when we hit u64::MAX in anything? It's overwhelmingly unlikely that we'll hit this, of course, but just a thought. Not sure if we do any math anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the sequence number space is that large, chances are that there will always be gaps in tests.
Fixed: 24ec63e.
But there will be gaps, regardless. We should fix test/validation - currently no priority bundles make it through the inclusion phase. But consider this out-of-scope for this PR.
Addendum: do we hit any tricky behavior when we hit u64::MAX in anything? It's overwhelmingly unlikely that we'll hit this, of course, but just a thought. Not sure if we do any math anywhere.
iiuc it is the responsibility of the PLC to ensure correct bundle format (incl. correct seqno). We could do saturating/checked add in places like:
timeboost/timeboost-sequencer/src/include.rs
Line 172 in 24ec63e
.any(|(x, y)| *x + 1 != *y) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments
let data = dec | ||
.get(i + ptxs.len()) | ||
.get(i + priority_bundles.len()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're touching this, it might be nice to get a comment here. Not sure what this offset is doing with i + len
. Mind throwing something here to make drive-by edits easier?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a comment to the function to make it easier to parse.
@@ -21,7 +24,7 @@ pub struct Includer { | |||
/// Consensus delayed inbox index. | |||
index: DelayedInboxIndex, | |||
/// Cache of transaction hashes for the previous 8 rounds. | |||
cache: BTreeMap<RoundNumber, HashSet<Hash>>, | |||
cache: BTreeMap<RoundNumber, HashSet<[u8; 32]>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 32?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a 256-bit keccak hash.
timeboost-sequencer/src/queue.rs
Outdated
match b { | ||
BundleVariant::Regular(b) => inner.regular.push_back((now, b)), | ||
BundleVariant::Priority(b) => | ||
// TODO: Check auction contract address on bundle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case this happens when you're not here when we tackle this, can we get a bit more detail on this? I will make it a ticket as well. Basically, what are we checking? We can either document here, or can you please add a ticket in asana so I know what to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove it from here since the TODO is already present in Bundle::validate
.
The check in centralized timeboost: https://github.com/OffchainLabs/nitro/blob/1e16dc408d24a7784f19acd1e76a71daac528a22/execution/gethexec/express_lane_service.go#L414.
Applying Occam's razor it seems like an extra check in case of multiple auctions on the same chain.
.map(|p| (p.bundle(), true)) | ||
.chain(regular.iter().map(|r| (r, false))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come we aren't just using the Priority types here instead?
Err(err) => { | ||
warn!(%err, "failed to decode transaction") | ||
} | ||
} | ||
} | ||
} | ||
Err(err) => { | ||
warn!(?err, "failed to ssz-decode priority bundle") | ||
warn!(?err, "failed to ssz-decode bundle") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when this dies? Do we throw everything away? If it's a serious issue, perhaps it could be an error? Who is at fault when such a circumstance happens? The sender? Sailfish?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean by die. If ssz_decode results in an error then there is not much to do. The bundle will simply be ignored but hopefully all honest nodes will come to the same conclusion. imo warn
is an appropriate verbosity level and we can always adjust this.
self.epoch | ||
} | ||
|
||
pub fn data(&self) -> &Bytes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need trivial getters instead of just making the fields public?
&self.bundle | ||
} | ||
|
||
pub fn auction(&self) -> Address { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit for later, Address
and cliquenet::Address
are sometimes head-spinning if they crop up in the same file. Is this Address
here an ethereum address? Perhaps we can rename? Doon't do it here, just want thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure. rename sounds good.
Ok(()) | ||
} | ||
|
||
pub fn sender(&self) -> Result<Address, ValidationError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get a doc comment here? It's not super clear why this is a failible operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
timeboost-types/src/bundle.rs
Outdated
) -> arbitrary::Result<PriorityBundle<Signed>> { | ||
let bundle = Bundle::arbitrary(u)?; | ||
let auction = Address::default(); | ||
let seqno = SeqNo::from(u.int_in_range(1..=u64::MAX)?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addendum: do we hit any tricky behavior when we hit u64::MAX in anything? It's overwhelmingly unlikely that we'll hit this, of course, but just a thought. Not sure if we do any math anywhere.
[route.submit-priority] | ||
PATH = ["/submit-priority"] | ||
METHOD = "POST" | ||
DOC = "Submit transaction to Timeboost" | ||
DOC = "Submit priority bundle to Timeboost" | ||
|
||
[route.submit-regular] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? Would a user ever submit a priority bundle to the node? Doesn't the user just submit a transaction and only the PLC has priority tx? How do we not spoof this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you envision a PLC submitting priority bundles if not through an API endpoint? The bundle will eventually be authenticated (PLC signature will be checked) before being queued.
timeboost-types/Cargo.toml
Outdated
@@ -5,6 +5,9 @@ version.workspace = true | |||
edition.workspace = true | |||
rust-version.workspace = true | |||
|
|||
[features] | |||
default = ["arbitrary"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than making this test-related feature a default I think the relevant code sections should be feature-gated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. That is indeed better. Fixed in e855b6f.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This PR is an implementation of the Bundle-centric approach for Timeboost transactions and roughly follows the design proposal (see Design Proposal for detailed description).
tl;dr:
Content
This PR refactors timeboost phases to handle the new format for (regular)
Bundle
and modifiedPriorityBundle
. Most of the underlying logic remains unchanged.Other points