-
Notifications
You must be signed in to change notification settings - Fork 132
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
Sync ethereum headers using unsigned (substrate) transactions #45
Conversation
For those who want to test it, I'm using following script: #!/bin/bash
set -e
rm -rf substrate.db
rm -rf parity.db
cargo build --manifest-path=../parity-bridges-common/Cargo.toml -p bridge-node
cp ../parity-bridges-common/target/debug/bridge-node .
cargo build --manifest-path=../parity-bridges-common/Cargo.toml -p ethereum-poa-relay
cp ../parity-bridges-common/target/debug/ethereum-poa-relay .
unbuffer ./parity -d parity.db --chain kovan --no-warp --no-persistent-txqueue --jsonrpc-apis=all 2>&1 | unbuffer -p gawk '{ print strftime("Parity: [%Y-%m-%d %H:%M:%S]"), $0 }' | unbuffer -p tee parity.log&
unbuffer ./bridge-node --dev -d parity.db 2>&1 | unbuffer -p gawk '{ print strftime("Substrate: [%Y-%m-%d %H:%M:%S]"), $0 }' | unbuffer -p tee substrate.log&
sleep 10
unbuffer ./ethereum-poa-relay --sub-tx-mode=unsigned 2>&1 | unbuffer -p gawk '{ print strftime("Bridge: [%Y-%m-%d %H:%M:%S]"), $0 }' | unbuffer -p tee bridge.log&
|
|
||
// we do not want to see more at most one header with given number from single authority | ||
// => every header is providing tag (block_number + authority) | ||
// => since only one tx in the pool can provide the same tag, they're auto-deduplicated |
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 only for the pool of unsigned transactions right? Also, are "tags" specific to unsigned transactions?
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.
Yes - this function is called only for unsigned transactions. I think tags are also valid for signed transactions, but they're provided at some other level (probably when runtime checks if we're able to dispatch call?). Better ask @tomusdrw .
And another question for @tomusdrw - how's pool/runtime deals with tags collisions? Or it is the runtime author problem (then we probably want to include method name in the tag)? Like if I have two completely unrelated methods (one is endow_account()
and the second one is update_timestamp()
). Both are providing/require tags which are encoded u32
=> could it be that (by occasion) I'll move future endow_account()
transaction to ready queue when some update_timestamp()
transaction is mined (because it provides required tag (42u32.encode()
)? TBH I wasn't thinking about that yet, so my tags are simply something like (number, hash).encode()
.
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.
Re 1: The tags for signed transactions are generated by frame_system
based on the sender and nonce (see CheckNonce
SignedExtension
).
Re 2: Indeed, we need to guarantee uniqueness of provides
tags to make sure they don't conflict between pallets and function calls. Simply including some pallet-specific prefix as string should do. Suggested in another comment.
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.
lgtm! Few grumbles
|
||
// we do not want to see more at most one header with given number from single authority | ||
// => every header is providing tag (block_number + authority) | ||
// => since only one tx in the pool can provide the same tag, they're auto-deduplicated |
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.
Re 1: The tags for signed transactions are generated by frame_system
based on the sender and nonce (see CheckNonce
SignedExtension
).
Re 2: Indeed, we need to guarantee uniqueness of provides
tags to make sure they don't conflict between pallets and function calls. Simply including some pallet-specific prefix as string should do. Suggested in another comment.
@@ -53,6 +53,9 @@ const SUBSTRATE_TICK_INTERVAL_MS: u64 = 5_000; | |||
/// the subscriber will receive every best header (2) reorg won't always lead to sync | |||
/// stall and restart is a heavy operation (we forget all in-memory headers). | |||
const STALL_SYNC_TIMEOUT_MS: u64 = 30_000; | |||
/// Delay (in milliseconds) after we have seen update of best Ethereum header in Substrate, | |||
/// for us to treat sync stalled. ONLY when relay operates in backup mode. | |||
const BACKUP_STALL_SYNC_TIMEOUT_MS: u64 = 5 * 60_000; |
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 stuff should probably be configurable as it might be chain-specific no? It's fine for now though.
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.
Yeah - there are some constants that are chain specific. We could either make them configurable through cli, or probably auto-adjust, based on what we are receiving from clients. I'll file an issue later
Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
…tytech/parity-bridges-common into unsigned-eth-headers-submit
Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
Re tag prefixes + configurable priority/longevity - let's extract this to separate issue, because there are also other things that needs to be extracted and configured from outside. Most important are aura configuration and validators configuration. Now we can only sync Kovan using this module. P.S.: do not forget about #45 (comment) |
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.
lgtm!
…tech#45) * reward submitters on finalization * PoA -> Substrate: unsigned_import_header API * fix grumble * make submitter part of ImportContext * verify using next validators set + tests * fix nostd compilation * add sub-tx-mode argument * support sub-tx-mode * impl ValidateUnsigned for Runtime * do not submit too much transactions to the pool * cargo fmt * fix bad merge * revert license fix * Update modules/ethereum/src/lib.rs Co-Authored-By: Hernando Castano <HCastano@users.noreply.github.com> * Update modules/ethereum/src/verification.rs Co-Authored-By: Hernando Castano <HCastano@users.noreply.github.com> * updated comment * validate receipts before accepting unsigned tx to pool * cargo fmt * fix comment * fix grumbles * Update modules/ethereum/src/verification.rs Co-Authored-By: Hernando Castano <HCastano@users.noreply.github.com> * cargo fmt --all * struct ChangeToEnact * updated doc * fix doc * add docs to the code method * simplify fn ancestry * finish incomplete docs * Update modules/ethereum/src/lib.rs Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * Update modules/ethereum/src/lib.rs Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * return err from unsigned_import_header * get header once * Update relays/ethereum/src/ethereum_sync.rs Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * fix * UnsignedTooFarInTheFuture -> Custom(err.code()) * updated ImportContext::last_signal_block * cargo fmt --all * rename runtime calls Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
…tech#45) * reward submitters on finalization * PoA -> Substrate: unsigned_import_header API * fix grumble * make submitter part of ImportContext * verify using next validators set + tests * fix nostd compilation * add sub-tx-mode argument * support sub-tx-mode * impl ValidateUnsigned for Runtime * do not submit too much transactions to the pool * cargo fmt * fix bad merge * revert license fix * Update modules/ethereum/src/lib.rs Co-Authored-By: Hernando Castano <HCastano@users.noreply.github.com> * Update modules/ethereum/src/verification.rs Co-Authored-By: Hernando Castano <HCastano@users.noreply.github.com> * updated comment * validate receipts before accepting unsigned tx to pool * cargo fmt * fix comment * fix grumbles * Update modules/ethereum/src/verification.rs Co-Authored-By: Hernando Castano <HCastano@users.noreply.github.com> * cargo fmt --all * struct ChangeToEnact * updated doc * fix doc * add docs to the code method * simplify fn ancestry * finish incomplete docs * Update modules/ethereum/src/lib.rs Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * Update modules/ethereum/src/lib.rs Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * return err from unsigned_import_header * get header once * Update relays/ethereum/src/ethereum_sync.rs Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com> * fix * UnsignedTooFarInTheFuture -> Custom(err.code()) * updated ImportContext::last_signal_block * cargo fmt --all * rename runtime calls Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
closes #26
I've also updated relay to support 3 modes (to be able to test this + to be able to use it as 'backup' option):
./ethereum-poa-relay --sub-tx-mode=signed
: default mode, sync using signed transactions../ethereum-poa-relay --sub-tx-mode=unsigned
: sync using unsigned transactions (slow when compared to signed, because we allow max 10 transactions (read: eth headers) in pool)./ethereum-poa-relay --sub-tx-mode=backup
: sync using signed transactions, but only when we do not see any new eth headers in substrate chain for 5 minutes (it should work like burst: if no new headers => submit 128 eth headers to substrate, then wait again for 5min delay).I've extracted #38 into separate issue - will do that next