Skip to content
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

Pass structured event log to ethereumInboundQueue.submit #1003

Merged
merged 27 commits into from
Nov 12, 2023
Merged

Conversation

vgeddes
Copy link
Collaborator

@vgeddes vgeddes commented Nov 9, 2023

We were making life difficult for ourselves by passing the message data (event log) as raw bytes. Makes troubleshooting harder, and also increases complexity of on-chain unnecessarily, as it has to decode the bytes. This also brings with it a small security risk, if the decoding can be exploited somehow.

I have also updated the register_token smoketest to pretty-print the event log (thanks @yrong for initiating that).

// Generated from smoketests:
//   cd smoketests
//   ./make-bindings
//   cargo test --test register_token -- --nocapture
fn mock_event_log() -> Log {
	Log {
		// gateway address
		address: hex!("eda338e4dc46038493b885327842fd3e301cab39").into(),
		topics: vec![
			hex!("5066fbba677e15936860e04088ca4cad3acd4c19706962196a5346f1457f7169").into(),
			// destination parachain id
			hex!("00000000000000000000000000000000000000000000000000000000000003e8").into(),
			// message id
			hex!("afad3c9777134532ae230b4fad334eef2e0dacbb965920412a7eaa59b07d640f").into(),
		],
		// Nonce + Payload
		data: hex!("00000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000001e000f000000000000000087d1f7fdfee7f651fabc8bfcb6e086c278b77a7d0000").into(),
	}
}

The only issue is that our codebase now has two Log types:

  1. snowbridge_core::inbound::Log
  2. snowbridge_ethereum::log::Log

I didn't standardize on snowbridge_ethereum::log::Log, as that crate uses various ethereum dependencies that are kinda deprecated and unmaintained. Rust folks are now using this library for ethereum types: https://github.com/alloy-rs/core. Anyway, this older Log version is still used by the beacon light client.

So I created snowbridge_core::inbound::Log, which is only used by the inbound-queue crate, and converted to snowbridge_ethereum::log::Log when needed.

@vgeddes vgeddes changed the base branch from main to message-id November 9, 2023 14:44
Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Attention: 47 lines in your changes are missing coverage. Please review.

Comparison is base (618e61b) 81.13% compared to head (64dac6d) 81.14%.
Report is 2 commits behind head on main.

❗ Current head 64dac6d differs from pull request most recent head e23ac89. Consider uploading reports for the commit e23ac89 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1003   +/-   ##
=======================================
  Coverage   81.13%   81.14%           
=======================================
  Files          52       53    +1     
  Lines        2115     2132   +17     
  Branches       72       72           
=======================================
+ Hits         1716     1730   +14     
- Misses        384      387    +3     
  Partials       15       15           
Flag Coverage Δ
rust 81.20% <58.40%> (+0.01%) ⬆️

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

Files Coverage Δ
...arachain/pallets/ethereum-beacon-client/src/lib.rs 96.22% <ø> (ø)
...rachain/pallets/ethereum-beacon-client/src/mock.rs 100.00% <100.00%> (ø)
parachain/pallets/inbound-queue/src/envelope.rs 100.00% <100.00%> (ø)
parachain/primitives/core/src/inbound.rs 100.00% <100.00%> (ø)
parachain/primitives/ethereum/src/log.rs 75.00% <ø> (-8.34%) ⬇️
parachain/primitives/ethereum/src/receipt.rs 79.31% <ø> (ø)
parachain/primitives/router/src/outbound/mod.rs 90.09% <ø> (ø)
parachain/pallets/inbound-queue/src/lib.rs 83.05% <90.90%> (+0.59%) ⬆️
...achain/pallets/ethereum-beacon-client/src/impls.rs 57.89% <61.53%> (+2.08%) ⬆️
parachain/primitives/router/src/inbound/mod.rs 49.39% <43.83%> (-6.56%) ⬇️

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

Base automatically changed from message-id to main November 9, 2023 15:55
use sp_runtime::RuntimeDebug;
use sp_std::vec::Vec;

/// A trait for verifying inbound messages from Ethereum.
pub trait Verifier {
fn verify(message: &Message) -> Result<(), VerificationError>;
fn verify(event: &Log, proof: &Proof) -> Result<(), VerificationError>;
Copy link
Contributor

@yrong yrong Nov 10, 2023

Choose a reason for hiding this comment

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

Maybe not directly relevant to change in this PR, I would assume event is not required for the verification.

With the proof and header.receipts_root we can generate the receipt which internally already contains the event log.

let receipt = match Self::verify_receipt_inclusion(header.receipts_root, &message.proof) {

pub struct Receipt {
pub post_state_or_status: Vec<u8>,
pub cumulative_gas_used: u64,
pub bloom: Bloom,
pub logs: Vec<Log>,
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's a good observation. I saw that as well while doing this PR.

However I think its easier to troubleshoot problems if the Log is submitted as well. Otherwise if there are issues, we would have to decode the Proof manually to extract the Log. Which brings us full circle to the problem this PR is solving - improving debuggability.

@vgeddes vgeddes merged commit 8e921b2 into main Nov 12, 2023
3 of 5 checks passed
@vgeddes vgeddes deleted the message-log branch November 12, 2023 14:15
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.

2 participants