Remove minimal spec #1119

claravanstaden · 2024-01-17T18:04:03Z

remove beacon-minimal-spec feature
update lodestar to use mainnet spec when running the local net
test smoke tests work
investigate error: thread 'tests::submit_update_execution_headers_too_far_behind' has overflowed its stack
update test fixture/benchmark data
adds script to generate test data for inbound message queue tests (will aid Adds runtime tests #1114)
generate next sync committee update
check benchmarks work
check beefy relayer starting

Polkadot-sdk companion: Snowfork/polkadot-sdk#100

yrong · 2024-01-18T01:14:46Z

investigate error: thread 'tests::submit_update_execution_headers_too_far_behind' has overflowed its stack

Maybe you can try this env setup first

snowbridge/.github/workflows/parachain.yml

Line 100 in 2710a25

RUST_MIN_STACK: 8388608

yrong · 2024-01-18T01:27:39Z

web/packages/test/scripts/deploy-ethereum.sh

+    # if were are running locally speed up the seconds per slot from 12 seconds to 2 seconds. if we are not
+    # running locally, revert
+    if [ "$eth_network" == "localhost" ]; then
+        set_slot_time 12 2


Maybe slot time can be further decreased to 1 second, then a sync period will be 2.27 hours.
Anyway, it seems still a bit challenging to test the switchover process.

I would be careful being too aggressive, as it may mean the beacon-light-client could struggle to import headers at such a fast rate. May need relayer updates too.

It looks like 1 second per slot is fine for our local setup. The bridge hub parachain sometimes drops imports but then recovers. I think that's okay, for the sake of faster tests? Otherwise we can change the SECONDS_PER_SLOT back to 2 seconds.

claravanstaden · 2024-01-18T06:46:02Z

investigate error: thread 'tests::submit_update_execution_headers_too_far_behind' has overflowed its stack

Maybe you can try this env setup first

snowbridge/.github/workflows/parachain.yml

Line 100 in 2710a25

RUST_MIN_STACK: 8388608

Thanks! This did the trick. It will be a problem when running the test in polkadot-sdk, perhaps. It is maybe worthwhile to change the test to fit in the stack size, maybe use heap-allocation instead.

codecov · 2024-01-18T07:54:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d65afb4) 75.09% compared to head (79b8a30) 76.04%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1119      +/-   ##
==========================================
+ Coverage   75.09%   76.04%   +0.95%     
==========================================
  Files          58       58              
  Lines        2461     2455       -6     
  Branches       72       72              
==========================================
+ Hits         1848     1867      +19     
+ Misses        596      571      -25     
  Partials       17       17

Flag	Coverage Δ
rust	`75.15% <100.00%> (+1.13%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yrong · 2024-01-21T14:17:12Z

parachain/pallets/inbound-queue/src/benchmarking/fixtures.rs

+// Generated, do not edit!
+// See ethereum client README.md for instructions to generate
 use hex_literal::hex;
 use snowbridge_beacon_primitives::CompactExecutionHeader;
 use snowbridge_core::inbound::{Log, Message, Proof};
 use sp_std::vec;

 pub struct InboundQueueTest {
-	pub execution_header: CompactExecutionHeader,
-	pub message: Message,
+    pub execution_header: CompactExecutionHeader,
+    pub message: Message,
 }


Cool! IIUC now the InboundQueueFixture is also auto-generated based on a template file.

I would like it's not limited to benchmark tests and also apply to unit/integration tests, more context in #1115 (comment).

I agree, I think we can use these fixtures in more tests. I thought about moving these tests fixtures into a separate crate so we can easily reference them.

claravanstaden · 2024-01-19T13:27:31Z

.github/workflows/parachain.yml

-      # Run tests for fast-runtime feature
-      - name: Tests for fast-runtime
-        working-directory: parachain
-        run: >
-          cargo test
-          --workspace
-          --features fast-runtime


Fast runtime isn't necessary for the ethereum client anymore, since the runtime is sped up outside of the spec.

claravanstaden · 2024-01-19T13:27:33Z

parachain/pallets/ethereum-client/Cargo.toml

@@ -95,5 +95,3 @@ try-runtime = [
 	"pallet-timestamp?/try-runtime",
 	"sp-runtime/try-runtime",
 ]
-beacon-spec-minimal = []


No more this annoying feature...

claravanstaden · 2024-01-19T13:31:38Z

parachain/pallets/ethereum-client/tests/fixtures/inbound-message.json

+  "execution_header": {
+    "parent_hash": "0x088df21dc48b1ef18b6df9ef35dc3b21eda78f943813436d5059fb3b8248c74a",
+    "state_root": "0xc1e042d99f2e5d21f4be14cca504ce8bd961db18084a1908431686ef918900bd",
+    "receipts_root": "0x7b1f61b9714c080ef0be014e01657a15f45f0304b477beebc7ca5596c8033095",
+    "block_number": 210
+  },
+  "message": {
+    "event_log": {
+      "address": "0xeda338e4dc46038493b885327842fd3e301cab39",
+      "topics": [


I added generating the inbound message test fixture to the beacon data fixture generation, since it is kinda of related. This message was generated on the same run as the beacon test fixture data so we can now write a test in #1114 that:

sets the beacon checkpoint

submit a sync committee update

submit a finalized header

submit an execution header containing a message

verify the message

claravanstaden · 2024-01-21T19:35:58Z

parachain/pallets/inbound-queue/src/benchmarking/fixtures.rs

+// Generated, do not edit!
+// See ethereum client README.md for instructions to generate
 use hex_literal::hex;
 use snowbridge_beacon_primitives::CompactExecutionHeader;
 use snowbridge_core::inbound::{Log, Message, Proof};
 use sp_std::vec;

 pub struct InboundQueueTest {
-	pub execution_header: CompactExecutionHeader,
-	pub message: Message,
+    pub execution_header: CompactExecutionHeader,
+    pub message: Message,
 }


I agree, I think we can use these fixtures in more tests. I thought about moving these tests fixtures into a separate crate so we can easily reference them.

claravanstaden · 2024-01-21T20:27:39Z

web/packages/test/scripts/build-binary.sh

-        fi
-        yarn install && yarn run build
-        popd
+    pushd $root_dir/lodestar


I think it's fine to build lodestar on every run because it won't rebuild if there are no changes.

Unfortunately, it did rebuild even no changes and cost almost 13s on my laptop.

Maybe that 13 seconds is OK? The reason why I would rather rebuild is, if you change fast runtime from true to false, lodestar won't rebuild.

Yeah, that's a problem. Maybe we can add a bool flag/env in the script to control whether to force rebuild or not. But it's very nit-picky so leave it up to you.

claravanstaden · 2024-01-21T20:27:59Z

web/packages/test/scripts/deploy-ethereum.sh

@@ -52,6 +52,8 @@ start_lodestar() {
            timestamp=$(date -d'+10second' +%s)
        fi

+        export LODESTAR_PRESET="mainnet"


Use the "mainnet" spec for local tests.

claravanstaden · 2024-01-21T20:28:43Z

flake.nix

@@ -37,9 +37,10 @@
                    ps

                    # typescript
+                    python3


Python is necessary to build lodestar.

claravanstaden · 2024-01-21T20:29:27Z

flake.nix

+                    nodejs_20
+                    (yarn.override { nodejs = nodejs_20; })


Yarn comes with node 18 so we need to override it here. Lodestar 1.12.0 doesn't build with 18.16.1

Btw the lodestar in our repo is the forked branch https://github.com/Snowfork/lodestar/tree/snowbridge which is based on 1.14.0 with a hack to work with node 18.16.

Anyway, the hack here is necessary for working with node 20 so happy to see it.

yrong

Awesome!

vgeddes

+1, looks great 🚀

claravanstaden mentioned this pull request Jan 17, 2024

Remove minimal spec Snowfork/polkadot-sdk#100

Merged

yrong reviewed Jan 18, 2024

View reviewed changes

claravanstaden force-pushed the remove-minimal-spec branch from 80ab7ce to af40733 Compare January 18, 2024 06:09

claravanstaden force-pushed the remove-minimal-spec branch 2 times, most recently from d97d1e6 to d8b035b Compare January 19, 2024 18:42

yrong mentioned this pull request Jan 21, 2024

Disable system calls from xcm & Refactoring emulated tests #1115

Merged

yrong reviewed Jan 21, 2024

View reviewed changes

claravanstaden force-pushed the remove-minimal-spec branch from 8f5cd28 to 7b606dc Compare January 21, 2024 19:34

claravanstaden commented Jan 21, 2024

View reviewed changes

claravanstaden marked this pull request as ready for review January 21, 2024 20:38

claravanstaden requested review from vgeddes, yrong and alistair-singh January 21, 2024 20:38

yrong mentioned this pull request Jan 22, 2024

Schedule dencun on sepolia #1122

Merged

yrong approved these changes Jan 22, 2024

View reviewed changes

vgeddes approved these changes Jan 22, 2024

View reviewed changes

claravanstaden force-pushed the remove-minimal-spec branch from 040b7a5 to 32a9bc8 Compare January 23, 2024 07:32

claravanstaden added 10 commits January 23, 2024 13:17

start removing minimal spec

f0c873a

remove minimal spec

98662af

remove minimal spec

eec937f

update test modules

9d885ab

heap allocate test structs

c8ba7da

fmt

7a0d233

fix tests

0305c30

remove fast runtime test run

53f975f

rename fixtures

6c32a3b

fmt

0839aef

claravanstaden added 15 commits January 23, 2024 13:17

adds generating test data for inbound message and refresh test data

781dcd8

adds generating fixture data for inbound message and clippy

e0cd0e0

fixes for inbound queue benchmarks

7b14792

update test fixture

8ba2851

format and update polkadot-sdk

8a0f875

cleanup and update fork versions

c80cba9

update fixtures

1e825ce

fixes after merge

188ba63

more fixes

d274243

fix lodestar setup in nix, update fixtures with deneb fork

1614113

use fast runtime, default fast mode

143da4d

fix fixtures

7308239

fix fixtures

c41de10

update fixtures

fb67ceb

fix fork versions

f87f4ef

claravanstaden force-pushed the remove-minimal-spec branch from 2f261b2 to f87f4ef Compare January 23, 2024 11:18

update polkadot-sdk

79b8a30

claravanstaden merged commit 53a5522 into main Jan 23, 2024
9 checks passed

claravanstaden deleted the remove-minimal-spec branch January 23, 2024 11:36

claravanstaden mentioned this pull request Jan 23, 2024

Use Milagro-BLS and Eth-ABI from crates.io #1124

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove minimal spec #1119

Remove minimal spec #1119

claravanstaden commented Jan 17, 2024 •

edited

Loading

yrong commented Jan 18, 2024

yrong Jan 18, 2024 •

edited

Loading

vgeddes Jan 18, 2024

claravanstaden Jan 21, 2024

claravanstaden commented Jan 18, 2024

codecov bot commented Jan 18, 2024 •

edited

Loading

yrong Jan 21, 2024 •

edited

Loading

claravanstaden Jan 21, 2024

claravanstaden Jan 19, 2024

claravanstaden Jan 19, 2024

claravanstaden Jan 19, 2024

claravanstaden Jan 21, 2024

claravanstaden Jan 21, 2024

yrong Jan 22, 2024

claravanstaden Jan 22, 2024

yrong Jan 22, 2024

claravanstaden Jan 21, 2024

claravanstaden Jan 21, 2024

claravanstaden Jan 21, 2024

yrong Jan 22, 2024

yrong left a comment

vgeddes left a comment

Remove minimal spec #1119

Remove minimal spec #1119

Conversation

claravanstaden commented Jan 17, 2024 • edited Loading

yrong commented Jan 18, 2024

yrong Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claravanstaden commented Jan 18, 2024

codecov bot commented Jan 18, 2024 • edited Loading

Codecov Report

yrong Jan 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrong left a comment

Choose a reason for hiding this comment

vgeddes left a comment

Choose a reason for hiding this comment

claravanstaden commented Jan 17, 2024 •

edited

Loading

yrong Jan 18, 2024 •

edited

Loading

codecov bot commented Jan 18, 2024 •

edited

Loading

yrong Jan 21, 2024 •

edited

Loading