Snowbridge - Fix benchmarks, add fee multipler and move config to Ethereum module #252

claravanstaden · 2024-03-22T08:23:27Z

The Snowbridge Inbound Queue pallet benchmarks are failing:

2024-03-21 13:50:40 Running  benchmark: snowbridge_pallet_ethereum_client.submit_execution_header(0 args) 1/1 1/1    
Running benchmarks for snowbridge_pallet_inbound_queue to ./bridge-hub-kusama-weights/
2024-03-21 13:50:41 Starting benchmark: snowbridge_pallet_inbound_queue::submit    
2024-03-21 13:50:41 💫 Verifying message with block hash 0x3921…3a3f    
2024-03-21 13:50:41 💫 Receipt verification successful for 0x3921…3a3f    
2024-03-21 13:50:41 panicked at /home/bparity/.cargo/registry/src/index.crates.io-6f17d22bba15001f/snowbridge-pallet-inbound-queue-0.1.0/src/benchmarking/mod.rs:43:13:
Expected Ok(_). Got Err(
    Module(
        ModuleError {
            index: 80,
            error: [
                0,
                0,
                0,
                0,
            ],
            message: Some(
                "InvalidGateway",
            ),
        },
    ),
)    
Error: 
   0: Invalid input: Error executing and verifying runtime benchmark: Execution aborted due to trap: wasm trap: wasm `unreachable` instruction executed
      WASM backtrace:
      error while executing at wasm backtrace:
          0: 0x436fa6 - <unknown>!rust_begin_unwind
          1: 0x2969 - <unknown>!core::panicking::panic_fmt::hbb0c5dd9c7ebab99
          2: 0x329f54 - <unknown>!core::ops::function::FnOnce::call_once{{vtable.shim}}::h0374b352d74ea114
          3: 0x300e24 - <unknown>!<bridge_hub_kusama_runtime::Runtime as frame_benchmarking::utils::runtime_decl_for_benchmark::BenchmarkV1<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32,sp_runtime::traits::BlakeTwo256>,sp_runtime::generic::unchecked_extrinsic::UncheckedExtrinsic<sp_runtime::multiaddress::MultiAddress<<<sp_runtime::MultiSignature as sp_runtime::traits::Verify>::Signer as sp_runtime::traits::IdentifyAccount>::AccountId,()>,bridge_hub_kusama_runtime::RuntimeCall,sp_runtime::MultiSignature,(frame_system::extensions::check_non_zero_sender::CheckNonZeroSender<bridge_hub_kusama_runtime::Runtime>,frame_system::extensions::check_spec_version::CheckSpecVersion<bridge_hub_kusama_runtime::Runtime>,frame_system::extensions::check_tx_version::CheckTxVersion<bridge_hub_kusama_runtime::Runtime>,frame_system::extensions::check_genesis::CheckGenesis<bridge_hub_kusama_runtime::Runtime>,frame_system::extensions::check_mortality::CheckMortality<bridge_hub_kusama_runtime::Runtime>,frame_system::extensions::check_nonce::CheckNonce<bridge_hub_kusama_runtime::Runtime>,frame_system::extensions::check_weight::CheckWeight<bridge_hub_kusama_runtime::Runtime>,pallet_transaction_payment::ChargeTransactionPayment<bridge_hub_kusama_runtime::Runtime>,bridge_hub_kusama_runtime::BridgeRejectObsoleteHeadersAndMessages,bridge_runtime_common::refund_relayer_extension::RefundSignedExtensionAdapter<bridge_runtime_common::refund_relayer_extension::RefundBridgedParachainMessages<bridge_hub_kusama_runtime::Runtime,bridge_runtime_common::refund_relayer_extension::RefundableParachain<frame_support::instances::Instance1,bp_bridge_hub_polkadot::BridgeHubPolkadot>,bridge_runtime_common::refund_relayer_extension::RefundableMessagesLane<frame_support::instances::Instance1,bridge_hub_kusama_runtime::bridge_to_polkadot_config::AssetHubKusamaToAssetHubPolkadotMessagesLane>,bridge_runtime_common::refund_relayer_extension::ActualFeeRefund<bridge_hub_kusama_runtime::Runtime>,bridge_hub_kusama_runtime::bridge_to_polkadot_config::PriorityBoostPerMessage,bridge_hub_kusama_runtime::bridge_to_polkadot_config::StrRefundBridgeHubPolkadotMessages>>)>>>>::dispatch_benchmark::h500c8f48422e47bd
          4: 0x3c0b6a - <unknown>!Benchmark_dispatch_benchmark

This PR fixes the error (the Ethereum gateway contract address should be set in the benchmarks since it is verified in the inbound queue when a message is submitted) and moves the benchmark config into bridge_to_ethereum_config.rs.

Does not require a CHANGELOG entry

system-parachains/bridge-hubs/bridge-hub-polkadot/src/bridge_to_ethereum_config.rs

Co-authored-by: Branislav Kontur <[email protected]>

system-parachains/bridge-hubs/bridge-hub-kusama/src/bridge_to_ethereum_config.rs

system-parachains/bridge-hubs/bridge-hub-polkadot/src/bridge_to_ethereum_config.rs

system-parachains/bridge-hubs/bridge-hub-polkadot/src/lib.rs

…fix-snowbridge-benchmarks

snowbridge-core v0.1.0 -> v0.1.1 snowbridge-outbound-queue-runtime-api v0.1.0 -> v0.1.1 snowbridge-pallet-ethereum-client v0.1.0 -> v0.1.1 snowbridge-pallet-inbound-queue v0.1.0 -> v0.1.1 snowbridge-pallet-outbound-queue v0.1.0 -> v0.1.1 snowbridge-pallet-system v0.1.0 -> v0.1.1 pallet-xcm v8.0.3 -> v8.0.4

acatangiu · 2024-03-23T08:36:12Z

Snowfork#5 for bringing in latest fixes

bump snowfork and pallet-xcm deps

# Conflicts: # integration-tests/emulated/tests/assets/asset-hub-kusama/Cargo.toml

claravanstaden · 2024-03-25T09:07:14Z

@bkontur @acatangiu I just need to fix the failing bridge-hub-kusama integration test quick before we merge.

`create_agent` benchmark fix

system-parachains/bridge-hubs/bridge-hub-polkadot/src/bridge_to_ethereum_config.rs

system-parachains/bridge-hubs/bridge-hub-kusama/src/bridge_to_ethereum_config.rs

eskimor

Looks good, left a few nits/remarks.

eskimor · 2024-03-25T10:46:53Z

system-parachains/bridge-hubs/bridge-hub-kusama/src/bridge_to_ethereum_config.rs

+	>;
+	type WeightToFee = WeightToFee;
+	type LengthToFee = ConstantMultiplier<Balance, TransactionByteFee>;
+	type MaxMessageSize = ConstU32<2048>;


Magic number? At least some docs/comment about the reasoning would be good.

eskimor · 2024-03-25T10:47:39Z

system-parachains/bridge-hubs/bridge-hub-kusama/src/bridge_to_ethereum_config.rs

+	type Hashing = Keccak256;
+	type MessageQueue = MessageQueue;
+	type Decimals = ConstU8<12>;
+	type MaxMessagePayloadSize = ConstU32<2048>;


If this has to be the same as the MaxMessageSize, maybe a constant would be good?

Definitely agree that the magic numbers needs to be hoisted into documented constants. Though MaxMessagePayloadSize does have a different purpose to snowbridge_pallet_inbound_queue::MaxMessageSize.

inbound_queue::MaxMessageSize: Worst case message size (including proofs), used as a pricing parameter to ultimately determine bridging fees on the Ethereum side of the bridge.

outbound_queue::MaxMessagePayloadSize: Maximum allowed size of encoded message payload that is committed for delivery to Ethereum

eskimor · 2024-03-25T10:47:58Z

system-parachains/bridge-hubs/bridge-hub-kusama/src/bridge_to_ethereum_config.rs

+	type MessageQueue = MessageQueue;
+	type Decimals = ConstU8<12>;
+	type MaxMessagePayloadSize = ConstU32<2048>;
+	type MaxMessagesPerBlock = ConstU32<32>;


Another magic number.

Agreed, we should make this a documented comment. MaxMessagesPerBlock refers to the maximum number of outbound messages to Ethereum that can be committed per block.

system-parachains/bridge-hubs/bridge-hub-kusama/src/bridge_to_ethereum_config.rs

…fix-snowbridge-benchmarks

Fresh weights for Ethereum pallets

bkchr · 2024-03-25T13:11:02Z

@claravanstaden tests are now failing. Are you working on fixing this?

bkontur · 2024-03-25T13:20:01Z

@claravanstaden @vgeddes my guess is that, these fee amounts here and here are weight-dependent, I would maybe remove those amount checks e.g. && *amount == 50710000, because we will have to adjust them on every runtime upgrade or weights change.

(But checking locally, if it is about that)

vgeddes · 2024-03-25T14:42:10Z

@claravanstaden tests are now failing. Are you working on fixing this?

About to push a fix, just waiting on tests to run..

bkontur · 2024-03-25T14:53:40Z

@claravanstaden @vgeddes my guess is that, these fee amounts here and here are weight-dependent, I would maybe remove those amount checks e.g. && *amount == 50710000, because we will have to adjust them on every runtime upgrade or weights change.

(But checking locally, if it is about that)

here is my suggestion with working tests: Snowfork#8

ggwpez · 2024-03-25T15:04:11Z

We dont have a CI check for benchmarks? We have some on the SDK, should probably have them here as well.

bkontur · 2024-03-25T15:07:36Z

We dont have a CI check for benchmarks? We have some on the SDK, should probably have them here as well.

#197

Make emulated tests more resilient

Fix `send_weth_asset_from_asset_hub_to_ethereum` test asserts

…onstant

…stant

bkchr · 2024-03-25T16:14:06Z

@vgeddes please run cargo fmt --all

claravanstaden · 2024-03-25T18:38:41Z

/merge

fellowship-merge-bot · 2024-03-25T18:40:05Z

Enabled auto-merge in Pull Request

Available commands

/merge: Enables auto-merge for Pull Request
/merge cancel: Cancels auto-merge for Pull Request
/merge help: Shows this menu

For more information see the documentation

acatangiu · 2024-03-25T18:59:16Z

migration CI jobs were automatically cancelled for no apparent reason 🤷 let's see if it goes ahead with the merge or needs manual intervention from some gh polkadot-fellows org member

fix benchmarks and move config to ethereum bridge config

198c1e9

bkontur reviewed Mar 22, 2024

View reviewed changes

system-parachains/bridge-hubs/bridge-hub-polkadot/src/bridge_to_ethereum_config.rs Show resolved Hide resolved

bkontur mentioned this pull request Mar 22, 2024

1.2.0 Release #140

Closed

19 tasks

vgeddes and others added 2 commits March 22, 2024 13:33

Add genesis config for Snowbridge

ec96ff7

Update chain-spec-generator/src/system_parachains_specs.rs

c70892f

Co-authored-by: Branislav Kontur <[email protected]>

bkontur reviewed Mar 22, 2024

View reviewed changes

system-parachains/bridge-hubs/bridge-hub-kusama/src/bridge_to_ethereum_config.rs Outdated Show resolved Hide resolved

bkontur reviewed Mar 22, 2024

View reviewed changes

system-parachains/bridge-hubs/bridge-hub-polkadot/src/bridge_to_ethereum_config.rs Outdated Show resolved Hide resolved

move runtime inits to config file

b4ac868

bkontur reviewed Mar 22, 2024

View reviewed changes

system-parachains/bridge-hubs/bridge-hub-polkadot/src/lib.rs Outdated Show resolved Hide resolved

claravanstaden added 5 commits March 22, 2024 15:24

move snowbridge config to config file

e8bb09c

fix benchmarks and move config to ethereum bridge config

9114c04

move runtime inits to config file

58d8c88

move snowbridge config to config file

d6fa40c

Merge remote-tracking branch 'origin/fix-snowbridge-benchmarks' into …

92c5e46

…fix-snowbridge-benchmarks

claravanstaden marked this pull request as ready for review March 22, 2024 14:03

claravanstaden and others added 3 commits March 23, 2024 13:37

Merge pull request #5 from acatangiu/bump-snowfork-deps

c11a7f7

bump snowfork and pallet-xcm deps

Merge branch 'main' into fix-snowbridge-benchmarks

31599af

# Conflicts: # integration-tests/emulated/tests/assets/asset-hub-kusama/Cargo.toml

fee multiplier changes

a50b119

bkontur approved these changes Mar 25, 2024

View reviewed changes

acatangiu approved these changes Mar 25, 2024

View reviewed changes

claravanstaden changed the title ~~Fix Snowbridge benchmarks~~ Snowbridge - Fix benchmarks, add fee multipler and move config to Ethereum module Mar 25, 2024

claravanstaden and others added 4 commits March 25, 2024 11:53

update local fee

16a0ec6

fix using polkadot config in kusama constants

86dff7a

create_agent benchmark fix

897d272

Merge pull request #6 from bkontur/bko-fix-snowbridge-benchmarks

db924ea

`create_agent` benchmark fix

bkchr approved these changes Mar 25, 2024

View reviewed changes

system-parachains/bridge-hubs/bridge-hub-polkadot/src/bridge_to_ethereum_config.rs Outdated Show resolved Hide resolved

system-parachains/bridge-hubs/bridge-hub-kusama/src/bridge_to_ethereum_config.rs Outdated Show resolved Hide resolved

eskimor reviewed Mar 25, 2024

View reviewed changes

claravanstaden and others added 3 commits March 25, 2024 13:13

Merge remote-tracking branch 'origin/fix-snowbridge-benchmarks' into …

0241dd7

…fix-snowbridge-benchmarks

comment about max execution headers to keep

410f9c3

Merge pull request #7 from bkontur/bko-fresh-weights

f34a2e7

Fresh weights for Ethereum pallets

polkadot-fellows deleted a comment from fellowship-merge-bot bot Mar 25, 2024

bkontur mentioned this pull request Mar 25, 2024

Add genesis config for Snowbridge #253

Closed

vgeddes approved these changes Mar 25, 2024

View reviewed changes

Fix send_weth_asset_from_asset_hub_to_ethereum test asserts

e674737

vgeddes added 5 commits March 25, 2024 17:29

Make emulated tests more resilient

73c8407

Merge pull request #9 from Snowfork/vincent/fix-snowbridge-benchmarks

71bb43a

Make emulated tests more resilient

Merge pull request #8 from bkontur/bko-snowbridge-itests

3671100

Fix `send_weth_asset_from_asset_hub_to_ethereum` test asserts

bridge-hub-polkadot: Update tests to use RelayTreasuryPalletAccount c…

c842fca

…onstant

bridge-hub-kusama: Update tests to use RelayTreasuryPalletAccount con…

9d62889

…stant

vgeddes and others added 5 commits March 25, 2024 18:15

rustfmt

48c69f3

unused import

83e392c

Merge branch 'main' into fix-snowbridge-benchmarks

8364a2c

update pallet-xcm version

1210322

Merge branch 'main' into fix-snowbridge-benchmarks

51149f9

fellowship-merge-bot bot enabled auto-merge (squash) March 25, 2024 18:40

fellowship-merge-bot bot merged commit bc9491d into polkadot-fellows:main Mar 25, 2024
33 of 34 checks passed

claravanstaden deleted the fix-snowbridge-benchmarks branch March 26, 2024 05:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snowbridge - Fix benchmarks, add fee multipler and move config to Ethereum module #252

Snowbridge - Fix benchmarks, add fee multipler and move config to Ethereum module #252

claravanstaden commented Mar 22, 2024

acatangiu commented Mar 23, 2024

claravanstaden commented Mar 25, 2024 •

edited

Loading

eskimor left a comment

eskimor Mar 25, 2024

eskimor Mar 25, 2024

vgeddes Mar 25, 2024 •

edited

Loading

eskimor Mar 25, 2024

vgeddes Mar 25, 2024

bkchr commented Mar 25, 2024

bkontur commented Mar 25, 2024

vgeddes commented Mar 25, 2024

bkontur commented Mar 25, 2024

ggwpez commented Mar 25, 2024

bkontur commented Mar 25, 2024

bkchr commented Mar 25, 2024

claravanstaden commented Mar 25, 2024

fellowship-merge-bot bot commented Mar 25, 2024

acatangiu commented Mar 25, 2024

Snowbridge - Fix benchmarks, add fee multipler and move config to Ethereum module #252

Snowbridge - Fix benchmarks, add fee multipler and move config to Ethereum module #252

Conversation

claravanstaden commented Mar 22, 2024

acatangiu commented Mar 23, 2024

claravanstaden commented Mar 25, 2024 • edited Loading

eskimor left a comment

Choose a reason for hiding this comment

eskimor Mar 25, 2024

Choose a reason for hiding this comment

eskimor Mar 25, 2024

Choose a reason for hiding this comment

vgeddes Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

eskimor Mar 25, 2024

Choose a reason for hiding this comment

vgeddes Mar 25, 2024

Choose a reason for hiding this comment

bkchr commented Mar 25, 2024

bkontur commented Mar 25, 2024

vgeddes commented Mar 25, 2024

bkontur commented Mar 25, 2024

ggwpez commented Mar 25, 2024

bkontur commented Mar 25, 2024

bkchr commented Mar 25, 2024

claravanstaden commented Mar 25, 2024

fellowship-merge-bot bot commented Mar 25, 2024

acatangiu commented Mar 25, 2024

claravanstaden commented Mar 25, 2024 •

edited

Loading

vgeddes Mar 25, 2024 •

edited

Loading