Make flashblock ws url as a flag and add more data #442
Conversation
|
Can you fix the checks? |
4c61581 to
d63eaca
Compare
| // there must be a line logging the monitoring transaction | ||
| op_rbuilder | ||
| .find_log_line("Committed block built by builder") | ||
| .find_log_line("Committed block built by builder") // no builder tx for flashblocks builder |
There was a problem hiding this comment.
Looks like the flashblock builder does not include this additional builder tx as the vanilla builder. https://github.com/flashbots/rbuilder/blob/develop/crates/op-rbuilder/src/payload_builder_vanilla.rs#L508
Is this expected? Curious what's the reason that there is always a builder tx for the vanilla builder
There was a problem hiding this comment.
the builder uses the builder tx for monitoring purposes, see here. These integration tests are for the vanilla builder, not flashblocks.
I think it would make more sense to have a different set of integration tests for flashblocks. I think you can configure this in rust like
// This test always runs
#[test]
fn always_runs() {
// Test code here
}
// This test only runs when "my_feature" is enabled
#[cfg(feature = "my_feature")]
#[test]
fn only_with_feature() {
// Test code that depends on my_feature
}
There was a problem hiding this comment.
Yeah it makes sense that these tests were built for the vanilla builder. I could definitely move these new tests logics to a new test under flashblocks feature.
But I wonder for op-rbuilder, shouldn't the flashblock builder be the default builder? It seems a bit weird to me that there's discrepancy for the two builders in terms of functionality, besides the subblock part
There was a problem hiding this comment.
flashblocks is still not ready for production so we are running the vanilla block builder as the default for now. I think eventually the plan is to make it default
|
|
||
| - name: Generate test genesis file | ||
| run: cargo run -p op-rbuilder --bin tester --features optimism -- genesis --output genesis.json | ||
|
|
||
| - name: Run integration tests | ||
| run: cargo test --package op-rbuilder --lib --features optimism,integration -- integration::integration_test::tests | ||
| run: cargo test --package op-rbuilder --lib --features optimism,integration,flashblocks -- integration::integration_test::tests |
There was a problem hiding this comment.
could you also add the features to the makefile?
There was a problem hiding this comment.
actually if you want the tests to pass you should remove flashblocks from the integration tests
There was a problem hiding this comment.
I think I should add another CI check for the flashblocks builder tests? (added above)
There was a problem hiding this comment.
if its guarded behind a feature flag you should need a separate command / CI test and everything should pass
There was a problem hiding this comment.
yup the above is a separate CI test in addition to the original test
📝 Summary
Making flashblock websocket as flag so that it can be customizable.
Also added receipts, account balance, block number into the metadata field, such that the RPC node can build the RPC response for
eth_transactionReceiptandeth_getBalancefor pending blocks. (danyalprout/reth-flashblocks#4)Added an integration test to test that the metadata field indeed has the new data.
✅ I have completed the following steps:
make lintmake test