Skip to content

Interop support#462

Merged
SozinM merged 2 commits intodevelopfrom
msozin/op-rbuilder/supervisor-1
Mar 18, 2025
Merged

Interop support#462
SozinM merged 2 commits intodevelopfrom
msozin/op-rbuilder/supervisor-1

Conversation

@SozinM
Copy link
Contributor

@SozinM SozinM commented Mar 5, 2025

Cherry-pick kona structs that we are using for interop validation
Implement SupervisorValidator to be used in rbuilder
Add additional primitives crate, that is used for storing external code

📝 Summary

💡 Motivation and Context


✅ I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

@SozinM SozinM force-pushed the msozin/op-rbuilder/supervisor-1 branch 4 times, most recently from 50098fc to 141120a Compare March 5, 2025 07:02
@SozinM
Copy link
Contributor Author

SozinM commented Mar 5, 2025

Don't look at the failing lint in this PR, it will be fixed with #465

@SozinM SozinM changed the title Interop support part 1 Interop support part 1: Deps Mar 5, 2025
@SozinM SozinM force-pushed the msozin/op-rbuilder/supervisor-1 branch from 29beca2 to feea7ae Compare March 6, 2025 09:17
@ZanCorDX ZanCorDX marked this pull request as draft March 7, 2025 14:15
@SozinM
Copy link
Contributor Author

SozinM commented Mar 11, 2025

@SozinM SozinM force-pushed the msozin/op-rbuilder/supervisor-1 branch 2 times, most recently from 82c927c to e3059a2 Compare March 13, 2025 06:18
@SozinM SozinM changed the title Interop support part 1: Deps Interop support Mar 13, 2025
Copy link

@emhane emhane left a comment

Choose a reason for hiding this comment

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

you're missing copyright license on a lot of files, read from copyright licence in kona:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

https://github.com/op-rs/kona/blob/main/LICENSE.md

Comment on lines 1141 to 1102
Err(err) => match err {
InteropTxValidatorError::SupervisorServerError(err) => {
warn!(target: "payload_builder", %err, ?sequencer_tx, "Supervisor error, skipping.");
self.metrics.inc_num_cross_chain_tx_server_error();
continue;
}
InteropTxValidatorError::ValidationTimeout(_) => {
trace!(target: "payload_builder", %err, ?sequencer_tx, "Executing message validation timed out, skipping.");
self.metrics.inc_num_cross_chain_tx_timeout();
continue;
}
err => {
trace!(target: "payload_builder", %err, ?sequencer_tx, "Executing message rejected.");
self.metrics.inc_num_cross_chain_tx_fail();
continue;
}
},
}
Copy link

Choose a reason for hiding this comment

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

there is also the client error which you probably want to match on specifically, because depending on what it is, you may want to try and reconnect with the supervisor

@SozinM
Copy link
Contributor Author

SozinM commented Mar 13, 2025

@emhane
Thanks! Right now i update rust version and could use kona itself and not copy the code, working on it now

@SozinM SozinM force-pushed the msozin/op-rbuilder/supervisor-1 branch 6 times, most recently from f66a68e to 69e0472 Compare March 13, 2025 10:57
pool,
ctx.provider().clone(),
Arc::new(BasicOpReceiptBuilder::default()),
self.supervisor_url.clone(),
Copy link

Choose a reason for hiding this comment

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

Suggested change
self.supervisor_url.clone(),
Some(ctx.provider()
.chain_spec()
.is_interop_active_at_timestamp(ctx.provider().chain_spec().genesis_header().timestamp())
.then(|| self.supervisor_url().ok_or(PayloadBuilderError::SupervisorUrlMissingPostInterop)?),

smthg like this

@SozinM SozinM force-pushed the msozin/op-rbuilder/supervisor-1 branch 4 times, most recently from c6e079a to 6d842fe Compare March 14, 2025 06:26
@SozinM SozinM marked this pull request as ready for review March 14, 2025 07:03
/// Returns (is_valid, is_recoverable)
pub fn is_cross_tx_valid<N: NodePrimitives>(
tx: &N::SignedTx,
client: Option<&SupervisorValidator>,
Copy link

Choose a reason for hiding this comment

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

Suggested change
client: Option<&SupervisorValidator>,
client: &SupervisorValidator,

it seems like the caller is responsible if checking if interop is activated, so supervisor client isn't an optional param

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to simplify execute_best_transactions as much as possible, it would look consistent
Don't the same for reth PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the reth PR I will ensure that the supervisor is set during the building, but in here we need to rebase first

Copy link

Choose a reason for hiding this comment

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

makes sense

///
/// If commitment present pre-interop tx rejected.
///
/// Returns (is_valid, is_recoverable)
Copy link

Choose a reason for hiding this comment

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

What exactly is is_recoverable? Is it like an RPC error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we remove totally invalid transactions from mempool
And if the transaction is not yet valid - just skip it for the current round

@tynes
Copy link

tynes commented Mar 14, 2025

I see that the supervisor client calls check_messages - it should be calling supervisor_checkAccessList per ethereum-optimism/specs#612

We probably want to enable a configurable safety level that is used in the client. The safety level could in theory be configured on a per chain or per message basis (accept any unsafe message if value is less than $100 for example) but that is def over engineering for now. I would be ok with either hardcoding the safety level to unsafe for now or allowing a runtime flag that allows it to be set to one of the possible values

@SozinM
Copy link
Contributor Author

SozinM commented Mar 17, 2025

@tynes thanks for review a lot!
We are using kona to validate messages and it's using supervisor_checkAccessList.
As for safety levels - before we were using cross-unsafe, and my experiences with unsafe were that any message (even non-existing) passes as unsafe, unless it grossly malformed, have it changed?
It's definitely a good idea to make it configurable, thank you!

Implement SupervisorValidator struct to use for validation
@SozinM SozinM force-pushed the msozin/op-rbuilder/supervisor-1 branch 2 times, most recently from 4ebb3ca to 237d2e3 Compare March 17, 2025 08:37
@SozinM SozinM force-pushed the msozin/op-rbuilder/supervisor-1 branch from 237d2e3 to 6a41b1b Compare March 17, 2025 08:56
@SozinM SozinM merged commit 586a2b2 into develop Mar 18, 2025
3 of 4 checks passed
@SozinM SozinM deleted the msozin/op-rbuilder/supervisor-1 branch March 18, 2025 08:31
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.

4 participants

Comments