Sassafras: align to the latest specs #4373

davxy · 2024-05-03T15:29:41Z

Changelog:

TicketIds are computed by on-chain code and not embedded within the TicketEnvelope
Submit tickets on-chain via inherent extrinsic
Discard block if extrinsic check fails for whatever reason (this is a consensus critical call)
Remove ephemeral keys from committed ticket body (rely on VRF output for claim)
A lot more efficient approach to tickets accumulation. Kept sorted using a map
Epoch index is just slot % epoch_len. Also we don't care about keeping this zero-based as it is only used internally
- first epoch length can thus be shorter than the others. But this is not an issue.
Protocol configuration is part of the pallet configuration and can't be changed via an extrinsic. We don't expect to see parameters changes and if in the future a change is required we can eventually update the runtime

Overall the code is a lot easier and less over-engineered

commit 265365920836bb1d286c9b48b1902a2de278fdd9 Author: Hernando Castano <[email protected]> Date: Wed Jan 29 19:51:15 2020 -0500 Move hc-jp-bridge repo to different folder commit 8271991e95320baba70bd1cb9c4234d0ffd5b638 Merge: 57d0811 304cbc5 Author: Hernando Castano <[email protected]> Date: Wed Jan 29 19:36:41 2020 -0500 Merge branch 'hc-jp-bridge-module' of hc-jp-bridge-module commit 304cbc5f02d003ffa5404c1c01e461e5b8539888 Author: Hernando Castano <[email protected]> Date: Wed Jan 29 00:38:27 2020 -0500 Update bridge pallet to work with the (almost) lastest master (paritytech#4672) * Update decl_error usage * WIP: Update error handling to use DispatchResult * Get module compiling with new error handling * Make tests compile again Main change was updating the usage of InMemoryBackend * Move `sp-state-machine` into dev-dependencies * Bump dependencies to v2.0.0 * Remove some stray comments * Appy code review suggestion commit 510cd6d96372688517496efa61773ea2839f8474 Author: Hernando Castano <[email protected]> Date: Tue Dec 17 12:52:51 2019 -0500 Move Bridge Pallet into FRAME (paritytech#4373) * Move `bridge` crate into `frame` folder * Make `bridge` pallet compile after `the-big-reorg` commit ab54e838ef75e6a3f68fd0944bf22598c10c552f Author: Hernando Castano <[email protected]> Date: Mon Nov 11 21:56:40 2019 +0100 Use new StorageProof type from paritytech#3834 commit 8fc8911fd1b4acc2274c6863fb3dba91b30c90af Author: Hernando Castano <[email protected]> Date: Tue Nov 5 00:50:34 2019 +0100 Verify Ancestry between Headers (paritytech#3963) * Create module for checking ancestry proofs * Use Vec of Headers instead of a HashMap * Move the ancestry verification into the lib.rs file * Change the proof format to exclude `child` and `ancestor` headers * Add a testing function for building header chains * Rename AncestorNotFound error to InvalidAncestryProof * Use ancestor hash instead of header when verifying ancestry * Clean up some stuff missed in the merge commit dbe85738b68358b790cf927b34a804b965a88f96 or: Hernando Castano <[email protected]> Date: Fri Nov 1 15:41:58 2019 +0100 Check given Grandpa validator set against set found in storage (paritytech#3915) * Make StorageProofChecker happy * Update some tests * Check given validator set against set found in storage * Use Finality Grandpa's Authority Id and Weight * Add better error handling * Use error type from decl_error! macro commit 31b09216603d3e9c21144ce8c0b6bf59307a4f97 or: Hernando Castano <[email protected]> Date: Wed Oct 23 14:55:37 2019 +0200 Make tests work after the changes introduced in paritytech#3793 (paritytech#3874) * Make tests work after the changes introduced in paritytech#3793 * Remove unneccessary import commit bce6d804aa86504599ff912387295c58f846cbf3 Author: Jim Posen <[email protected]> Date: Thu Oct 10 12:18:58 2019 +0200 Logic for checking Substrate proofs from within runtime module. (paritytech#3783) commit a7013e94b6c772c1d45a7cacbb445f73f6554fca Author: Hernando Castano <[email protected]> Date: Fri Oct 4 15:21:00 2019 +0300 Allow tracking of multiple bridges commit 3cf648242d631e32bd553a67df54bf5a48912839 Author: Hernando Castano <[email protected]> Date: Tue Oct 1 14:55:04 2019 +0200 Add BridgeId => Bridge mapping commit 001c74c45072213e01857d0a2454379b447c5a76 Author: Hernando Castano <[email protected]> Date: Tue Oct 1 11:10:19 2019 +0200 Get the mock runtime for tests set up commit 38443a1e8b424ed2f148eb95121d009f730e3b5a Author: Hernando Castano <[email protected]> Date: Fri Sep 27 14:52:53 2019 +0200 Clean up some warnings commit bdc3b01401e89c7111f8bf71f84c50750d25089f Author: Hernando Castano <[email protected]> Date: Thu Sep 26 16:41:01 2019 +0200 Add more skeleton code commit 26995efbf4bac2842eb2822322f7ad3c3e88feb8 Author: Hernando Castano <[email protected]> Date: Wed Sep 25 15:16:57 2019 +0200 Create `bridge` module skeleton

commit 657deb4cf4b90f24b9c5bfd62764b197776c262c Author: Hernando Castano <[email protected]> Date: Wed Jan 29 20:14:20 2020 -0500 Move Slava's bridge code into relays folder commit 4868c42c7da959dde7252766996b3ed4e408e439 Author: Hernando Castano <[email protected]> Date: Wed Jan 29 20:01:06 2020 -0500 Move files into `modules/ethereum` commit d1093f3e4238acb1a1a020011452cb928d3f8d7a Merge: 29dc6f9 bfd30ef Author: Hernando Castano <[email protected]> Date: Wed Jan 29 19:59:27 2020 -0500 Merge branch 'master' of slava-async-bridge commit 29dc6f97b1b7d1db99086d35a5336f43d2f0f8af Author: Hernando Castano <[email protected]> Date: Wed Jan 29 19:51:31 2020 -0500 Squashed commit of the following: commit 265365920836bb1d286c9b48b1902a2de278fdd9 Author: Hernando Castano <[email protected]> Date: Wed Jan 29 19:51:15 2020 -0500 Move hc-jp-bridge repo to different folder commit 8271991e95320baba70bd1cb9c4234d0ffd5b638 Merge: 57d0811 304cbc5 Author: Hernando Castano <[email protected]> Date: Wed Jan 29 19:36:41 2020 -0500 Merge branch 'hc-jp-bridge-module' of hc-jp-bridge-module commit 304cbc5f02d003ffa5404c1c01e461e5b8539888 Author: Hernando Castano <[email protected]> Date: Wed Jan 29 00:38:27 2020 -0500 Update bridge pallet to work with the (almost) lastest master (paritytech#4672) * Update decl_error usage * WIP: Update error handling to use DispatchResult * Get module compiling with new error handling * Make tests compile again Main change was updating the usage of InMemoryBackend * Move `sp-state-machine` into dev-dependencies * Bump dependencies to v2.0.0 * Remove some stray comments * Appy code review suggestion commit 510cd6d96372688517496efa61773ea2839f8474 Author: Hernando Castano <[email protected]> Date: Tue Dec 17 12:52:51 2019 -0500 Move Bridge Pallet into FRAME (paritytech#4373) * Move `bridge` crate into `frame` folder * Make `bridge` pallet compile after `the-big-reorg` commit ab54e838ef75e6a3f68fd0944bf22598c10c552f Author: Hernando Castano <[email protected]> Date: Mon Nov 11 21:56:40 2019 +0100 Use new StorageProof type from paritytech#3834 commit 8fc8911fd1b4acc2274c6863fb3dba91b30c90af Author: Hernando Castano <[email protected]> Date: Tue Nov 5 00:50:34 2019 +0100 Verify Ancestry between Headers (paritytech#3963) * Create module for checking ancestry proofs * Use Vec of Headers instead of a HashMap * Move the ancestry verification into the lib.rs file * Change the proof format to exclude `child` and `ancestor` headers * Add a testing function for building header chains * Rename AncestorNotFound error to InvalidAncestryProof * Use ancestor hash instead of header when verifying ancestry * Clean up some stuff missed in the merge commit dbe85738b68358b790cf927b34a804b965a88f96 or: Hernando Castano <[email protected]> Date: Fri Nov 1 15:41:58 2019 +0100 Check given Grandpa validator set against set found in storage (paritytech#3915) * Make StorageProofChecker happy * Update some tests * Check given validator set against set found in storage * Use Finality Grandpa's Authority Id and Weight * Add better error handling * Use error type from decl_error! macro commit 31b09216603d3e9c21144ce8c0b6bf59307a4f97 or: Hernando Castano <[email protected]> Date: Wed Oct 23 14:55:37 2019 +0200 Make tests work after the changes introduced in paritytech#3793 (paritytech#3874) * Make tests work after the changes introduced in paritytech#3793 * Remove unneccessary import commit bce6d804aa86504599ff912387295c58f846cbf3 Author: Jim Posen <[email protected]> Date: Thu Oct 10 12:18:58 2019 +0200 Logic for checking Substrate proofs from within runtime module. (paritytech#3783) commit a7013e94b6c772c1d45a7cacbb445f73f6554fca Author: Hernando Castano <[email protected]> Date: Fri Oct 4 15:21:00 2019 +0300 Allow tracking of multiple bridges commit 3cf648242d631e32bd553a67df54bf5a48912839 Author: Hernando Castano <[email protected]> Date: Tue Oct 1 14:55:04 2019 +0200 Add BridgeId => Bridge mapping commit 001c74c45072213e01857d0a2454379b447c5a76 Author: Hernando Castano <[email protected]> Date: Tue Oct 1 11:10:19 2019 +0200 Get the mock runtime for tests set up commit 38443a1e8b424ed2f148eb95121d009f730e3b5a Author: Hernando Castano <[email protected]> Date: Fri Sep 27 14:52:53 2019 +0200 Clean up some warnings commit bdc3b01401e89c7111f8bf71f84c50750d25089f Author: Hernando Castano <[email protected]> Date: Thu Sep 26 16:41:01 2019 +0200 Add more skeleton code commit 26995efbf4bac2842eb2822322f7ad3c3e88feb8 Author: Hernando Castano <[email protected]> Date: Wed Sep 25 15:16:57 2019 +0200 Create `bridge` module skeleton commit bfd30ef8363b1483ef1107ae1eb958a4e944c93b Author: Svyatoslav Nikolsky <[email protected]> Date: Tue Dec 10 12:10:53 2019 +0300 actually use signer from CLI to sign Substrate transactions commit 504028eac60d9d14ba95b506cd355b0d2f405ce0 Author: Svyatoslav Nikolsky <[email protected]> Date: Tue Dec 10 12:02:22 2019 +0300 go offline for a bit on connection error commit 446d0c8d20187dfd1beb173958ea28f2ad97887d Author: Svyatoslav Nikolsky <[email protected]> Date: Tue Dec 10 11:25:50 2019 +0300 enable info logs by default commit d039c60ec72bc91adfdad85442bc99a93b7f8e8d Author: Svyatoslav Nikolsky <[email protected]> Date: Tue Dec 10 11:12:51 2019 +0300 support basic CLI arguments commit 65c6d48e23576f36e8541878b920a03730226392 Author: Svyatoslav Nikolsky <[email protected]> Date: Mon Dec 9 15:37:48 2019 +0300 fix restart commit 96e94c1c4b22d732078f8c401b872c5f8246c3fe Author: Svyatoslav Nikolsky <[email protected]> Date: Mon Dec 9 14:57:53 2019 +0300 license commit 68f4191e6cdd211ac8975e0b79f8a6f46a3ca953 Author: Svyatoslav Nikolsky <[email protected]> Date: Mon Dec 9 14:56:05 2019 +0300 restart sync when Substrate reorgs && we are unlucky commit 29887c446167d580d73cc03a0b71c31890cafb51 Author: Svyatoslav Nikolsky <[email protected]> Date: Mon Dec 9 13:49:31 2019 +0300 only read genesis hash once commit 832492b8393fe2063adf9c58c2b9e060dc3e4efb Author: Svyatoslav Nikolsky <[email protected]> Date: Mon Dec 9 13:23:26 2019 +0300 changed TODO commit 9dbc130e5fa036ae63d973819daf30f4ed6ffb5b Author: Svyatoslav Nikolsky <[email protected]> Date: Mon Dec 9 13:16:56 2019 +0300 removed obsolete exit future commit d03408cd8284eb0c61e7e96429b4f6199353e030 Author: Svyatoslav Nikolsky <[email protected]> Date: Mon Dec 9 13:16:17 2019 +0300 removed obsolete TODOs + moved a couple of TODOs to runtime module commit ed8bec44b79f9a2ce829e59f10181368b2f42139 Author: Svyatoslav Nikolsky <[email protected]> Date: Mon Dec 9 12:37:05 2019 +0300 explained TODO fix commit aa9c4c66ec2904eeb6072d654718b0ac0b7d8803 Author: Svyatoslav Nikolsky <[email protected]> Date: Mon Dec 9 12:28:09 2019 +0300 fix tx outcome serialization commit 126f8f5484dac8c4af588ae86dc8855919d6c822 Author: Svyatoslav Nikolsky <[email protected]> Date: Mon Dec 9 12:05:05 2019 +0300 prune old ethereum headers when Substrate best header is too far in the future commit c7bd301e631a44fe3263e188d0956081aa84f31e Author: Svyatoslav Nikolsky <[email protected]> Date: Fri Dec 6 12:51:50 2019 +0300 fix trace commit 549bb7acdb30cfdafe6c8600f0410212539ea63d Author: Svyatoslav Nikolsky <[email protected]> Date: Fri Dec 6 12:51:26 2019 +0300 tx hashes are already a part of Block response commit 7864017909f87ea36955d605a924c3c88bc88df3 Author: Svyatoslav Nikolsky <[email protected]> Date: Thu Dec 5 12:29:37 2019 +0300 submit bunch of headers at once + some fixes commit 96485f85d38c144f0771f02ba692216a60356665 Author: Svyatoslav Nikolsky <[email protected]> Date: Wed Dec 4 17:22:13 2019 +0300 print status messages commit ae0ec4c087136db653339537daab7f96a8c21b65 Author: Svyatoslav Nikolsky <[email protected]> Date: Wed Dec 4 17:06:00 2019 +0300 continue actual Substrate client implementation commit 8146293740d70b88904568ff8e5acdfbadf06fd3 Author: Svyatoslav Nikolsky <[email protected]> Date: Wed Dec 4 13:49:30 2019 +0300 fix IncompleteHeader condition commit 767c6201157dabcccf7f62e643681ca298224fb1 Author: Svyatoslav Nikolsky <[email protected]> Date: Wed Dec 4 10:55:06 2019 +0300 actual Substrate client implementation commit 221fd4ccd2b1eea12c9dacf800d80e15ec115c1b Author: Svyatoslav Nikolsky <[email protected]> Date: Wed Nov 20 17:28:13 2019 +0300 initial commit

davxy · 2024-05-13T13:00:40Z

Issue for open tasks #2364

substrate/frame/sassafras/src/lib.rs

skunert

Made a first pass, looks really nice!

substrate/primitives/consensus/sassafras/src/ticket.rs

substrate/frame/sassafras/src/lib.rs

skunert · 2024-06-21T09:59:07Z

substrate/frame/sassafras/src/lib.rs

-	#[pallet::validate_unsigned]
-	impl<T: Config> ValidateUnsigned for Pallet<T> {
+	#[pallet::inherent]
+	impl<T: Config> ProvideInherent for Pallet<T> {


This could also implement fn is_inherent_required from ProvideInherent trait.

I left it to the default impl:

/// - `Ok(None)` indicates that this inherent is not required in this block. The default /// implementation returns this.

Inherents with tickets are never required. If a node has some tickets to share then he do it, but you don't know and is fine to don't know.

Maybe we can discuss about the DispatchClass:

#[pallet::call_index(0)] #[pallet::weight(( T::WeightInfo::submit_tickets(envelopes.len() as u32), DispatchClass::Mandatory ))] pub fn submit_tickets(origin: OriginFor<T>, envelopes: TicketsVec<T>) -> DispatchResult {

Currently I've set it as Mandatory.

polkadot-sdk/substrate/frame/support/src/dispatch.rs

Line 172 in a164639

pub enum DispatchClass {

Even though is said to avoid this for "heavy" operations. Even though this tickets verification is not the lightest of the operations, should be said that this kind of inherent is considered central to the consensus algorithm and if is there must be processed.
Should be said that there is an upper bound to the Tickets that can be processed in a single call, so the exact cost can be predicted (verification cost per ticket ~4ms on my machine https://github.com/davxy/crypto-benches/blob/main/vrf/README.md#verify).

skunert · 2024-06-21T12:40:04Z

substrate/frame/sassafras/src/lib.rs

-		let mut epoch_tag = (epoch_idx & 1) as u8;
+		let mut epoch_tag = slot_epoch_idx.tag();
+		let epoch_len = T::EpochLength::get();
+		let mut slot_idx = Self::slot_index(slot);

 		if epoch_len <= slot_idx && slot_idx < 2 * epoch_len {


Hmm I hope I am not missing something, but the way we compute slot_idx I think this branch is unreachable.

skunert · 2024-06-21T12:55:40Z

substrate/frame/sassafras/src/lib.rs

 			ensure_none(origin)?;

-			debug!(target: LOG_TARGET, "Received {} tickets", tickets.len());
+			debug!(target: LOG_TARGET, "Received {} tickets", envelopes.len());

 			let epoch_length = T::EpochLength::get();
 			let current_slot_idx = Self::current_slot_index();


Shouldn't we compute the tail one line below instead of epoch_length / 2?

Nice catch. I've haven't updated the logic after the introduction of configurable epoch length.

Now is not hardcoded as epoch_length/2 but we need to use the config param. (476b188)

skunert · 2024-06-21T12:58:46Z

substrate/frame/sassafras/src/lib.rs

-		Self::slot_ticket_id(slot).and_then(|id| TicketsData::<T>::get(id).map(|body| (id, body)))
-	}
+		let get_ticket_index = |slot_index: u32| {
+			let mut ticket_index = slot_idx / 2;


Suggested change

let mut ticket_index = slot_idx / 2;

let mut ticket_index = slot_index / 2;

I assume you want to use the parameter, not slot_idx captured from the outer scope

I simplified a bit the logic here. There were a couple of points that didn't make much sense :-)

Co-authored-by: Sebastian Kunert <[email protected]>

davxy · 2024-07-24T17:28:54Z

@skunert I think I've addressed all your notes

skunert

LGTM!

skunert · 2024-07-29T11:04:38Z

substrate/frame/sassafras/src/lib.rs

@@ -1071,7 +928,7 @@ impl EpochChangeTrigger for EpochChangeInternalTrigger {
 			let next_authorities = authorities.clone();
 			let len = next_authorities.len() as u32;
 			Pallet::<T>::enact_epoch_change(authorities, next_authorities);
-			T::WeightInfo::enact_epoch_change(len, T::EpochLength::get())
+			T::WeightInfo::enact_epoch_change(len, T::EpochDuration::get())
 		} else {
 			Weight::zero()


Was not changed in this PR, but the check for should_end_epoch is already not free as far as I see.

Begin Sassafras overhaul to align to the latest specs

d26386b

davxy self-assigned this May 3, 2024

davxy added the R0-silent Changes should not be mentioned in any release notes label May 3, 2024

davxy added 14 commits May 6, 2024 19:58

Rething epoch index management

f5ce73b

Configuration separated by current epoch data

fe461ba

More tests

2d08b1a

Test tickets submission and correct order for committed tickets

9aa0d12

Remove old implementation

680d88d

Fix warnings

1d0b093

Remove some leftovers

9e32ffb

Benchmarking

5249537

Weights update

edb1bdf

Fix test

af3f20a

Submit tickets via Inherents

7f91e75

Update lock file

457ee80

Merge branch 'master' into sassafras-overhaul

7cee2b6

Update lock

136673a

davxy marked this pull request as ready for review May 8, 2024 10:58

davxy requested a review from a team as a code owner May 8, 2024 10:58

Add license to weights

a421a58

davxy requested a review from a team May 8, 2024 11:03

Pre-computed tickets data

6d142e8

paritytech deleted a comment from paritytech-cicd-pr May 8, 2024

davxy added 5 commits May 9, 2024 11:20

Rename TicketsMetadata to TicketsCounter

61a4692

Update weights

5baff7b

Nits

257432f

Get rid of persistent epoch index

6c60e88

Improve docs

7ab117a

davxy added 2 commits May 10, 2024 19:53

Fix std feature propagation

6d8080d

Fallback to AURA if unable to construct ring verifier

72b1a65

davxy commented May 13, 2024

View reviewed changes

substrate/frame/sassafras/src/lib.rs Outdated Show resolved Hide resolved

davxy added 6 commits June 10, 2024 16:53

Ticket body is the onchain struct

dcb9042

Nit

683b7e5

Update benchmarking code

fe2f284

Merge branch 'master' into sassafras-overhaul

5d8b76b

Typo

fda29a6

Clippy

7a61b1c

davxy requested review from koute, skunert, bkchr, lexnv and dmitry-markin June 10, 2024 16:44

skunert reviewed Jun 21, 2024

View reviewed changes

davxy and others added 6 commits July 22, 2024 11:24

Remove dead code

d56c1c2

Merge branch 'master' into sassafras-overhaul

4bf4273

Fix after master merge

296380d

Apply suggestions from code review

a485f8e

Co-authored-by: Sebastian Kunert <[email protected]>

Rename epoch length -> duration

d3c3174

Update weights

712f307

paritytech deleted a comment from paritytech-cicd-pr Jul 24, 2024

davxy added 2 commits July 24, 2024 18:46

Simplify logic

f4ab66c

Excercise all failure paths during tickets submission

476b188

Update weights

20f57fe

davxy requested a review from a team July 24, 2024 17:35

Add note about epoch duration

bcfdd09

skunert approved these changes Jul 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sassafras: align to the latest specs #4373

Sassafras: align to the latest specs #4373

davxy commented May 3, 2024 •

edited

Loading

davxy commented May 13, 2024

skunert left a comment

skunert Jun 21, 2024

davxy Jul 24, 2024

skunert Jun 21, 2024

davxy Jul 24, 2024

skunert Jun 21, 2024

davxy Jul 24, 2024

skunert Jun 21, 2024

davxy Jul 24, 2024 •

edited

Loading

davxy commented Jul 24, 2024

skunert left a comment

skunert Jul 29, 2024

	let mut ticket_index = slot_idx / 2;
	let mut ticket_index = slot_index / 2;

Sassafras: align to the latest specs #4373

Are you sure you want to change the base?

Sassafras: align to the latest specs #4373

Conversation

davxy commented May 3, 2024 • edited Loading

davxy commented May 13, 2024

skunert left a 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

davxy Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

davxy commented Jul 24, 2024

skunert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davxy commented May 3, 2024 •

edited

Loading

davxy Jul 24, 2024 •

edited

Loading