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

Timestamp Inherent #100

Merged
merged 134 commits into from
Oct 13, 2023
Merged
Show file tree
Hide file tree
Changes from 117 commits
Commits
Show all changes
134 commits
Select commit Hold shift + click to select a range
771d02e
Sketch piece draft
Sep 27, 2023
36b5956
Builds, Runs, Logs look okay, but no inherent extrinsics in block.
Sep 27, 2023
dfa2fc7
comment wrong tests
Sep 27, 2023
86b2589
A few more logs. Seems like the authoring task is not putting inheren…
Sep 27, 2023
72d8d66
cargo fmt
Sep 27, 2023
e9f039b
Actually return the inherent extrinsic!
Sep 27, 2023
9b86b72
Explore Aura slot a little bit too. This will probably be removed bef…
Sep 27, 2023
3b57339
toml sort
Sep 27, 2023
821d99e
Sketch plan for constructing utxo-style inherent extrinsics.
Sep 28, 2023
75d3718
cargo fmt
Sep 28, 2023
78f9ee5
Sketch some actual transaction construction logic
Sep 28, 2023
698b725
Implement InherentDataProvider for tuxedo parent block inherent data …
Sep 28, 2023
7721424
wire parent block idp into service
Sep 28, 2023
04d0f19
Update piece to require consuming previous timestamp.
Sep 28, 2023
97c0530
off by one error
Sep 28, 2023
a0d806e
update piece for first block exception
Sep 28, 2023
ac2d722
cargo fmt
Sep 28, 2023
39d9001
Attempt at checking timestamp inherent. Not working yet.
Sep 28, 2023
95f3788
toml sort
Sep 29, 2023
01bdac4
correct max drift calculation.
Sep 29, 2023
d26afb3
Add logs to `check_inherents`
Sep 29, 2023
eace947
Better logs. I'm struggling with a multinode network. Not sure if its…
Sep 29, 2023
e73fa69
Use `StateVersion::V0` when calculating extrinsics root to match Subs…
Sep 29, 2023
375c9ce
Remove incorrect comment.
Sep 29, 2023
5bdd2a1
remove commented code
Sep 29, 2023
64f0630
Sketch some cleanup ideas.
Sep 29, 2023
9b2a00c
cargo fmt
Sep 29, 2023
5e4ad6b
Start sketching toward separate notions of best and noted time.
Sep 29, 2023
eb6f8e2
Actually distinguish `BestTimestamp` vs `NotedTimestamp` at the type …
Sep 29, 2023
059a89f
attempt to make clippy happy
Sep 29, 2023
d172b20
Merge branch 'main' into timestamp
Sep 29, 2023
858d9f3
cargo fmt
Sep 29, 2023
f3710e1
structure test suite
Oct 2, 2023
1ec0bef
Sketch actual test cases
Oct 2, 2023
b3c74fa
fmt
Oct 2, 2023
fbcc042
rename set -> update?
Oct 2, 2023
ffa2c9a
First working test
Oct 2, 2023
b729e6d
Fix previous best check
Oct 3, 2023
0545d85
moar update tests
Oct 3, 2023
f8983e7
moar update tests
Oct 3, 2023
632787e
Write out last of update timestamp tests.
Oct 3, 2023
8ecdba2
first cleanup test
Oct 3, 2023
8ed0943
moar cleanup tests
Oct 3, 2023
159df78
multiple cleanup tests
Oct 3, 2023
705624d
featuregate all test modules
Oct 3, 2023
4a97938
Finsih cleanup tests
Oct 3, 2023
2d395a0
test first block hack
Oct 3, 2023
c4b9fed
cargo fmt
Oct 3, 2023
e818e8c
forbid timestamp calls in pool
Oct 4, 2023
2f9441b
Sketch idea for `TuxedoInherent` trait, an attempt at a reusable inte…
Oct 4, 2023
05a98ff
fix warnings
Oct 4, 2023
fb2c449
Revise inherent interface and sketch into timestamp piece
Oct 4, 2023
ead4e98
move create into timestamp piece, a little more revising the trait
Oct 4, 2023
fb19665
add static to PoeConfig too. I'm not sure why this started coming up …
Oct 4, 2023
992f0e0
make shit compile
Oct 4, 2023
19881d5
cargo fmt
Oct 4, 2023
3c4b7f6
Fix off by one error
Oct 4, 2023
475878d
prune the aura inherent stuff. It was useful for learning
Oct 4, 2023
dd8d4bb
Revert "forbid timestamp calls in pool"
Oct 4, 2023
aaea5f9
Logging and generics fucking everywhere. This is gettinga little ugl…
Oct 4, 2023
4a8cdab
Move is_inherent to Constraint checker; implement is_signed
Oct 4, 2023
7e76ab8
cargo fmt
Oct 4, 2023
f7f31d1
move checking logic into timestamp piece
Oct 4, 2023
ec19f60
fix inherent identifier
Oct 5, 2023
fc7728c
rework internal trait to always use Vecs
Oct 9, 2023
d4bf5c4
start working on the gagregator. This is getting complicated.
Oct 9, 2023
276fec3
finish first draft of compiling aggregator.
Oct 10, 2023
355d748
First draft of executive compiles
Oct 10, 2023
6af2af8
cargo fmt
Oct 10, 2023
57b3fd5
Whole node compiles
Oct 10, 2023
63c00b8
Prune comment
Oct 10, 2023
d5b405f
First block hack in create flow. It actually runs and imports blocks!!!!
Oct 10, 2023
dd5cb36
Important TODO about making sure every expected inherent is present (…
Oct 10, 2023
68cfc6e
cargo fmt
Oct 10, 2023
c3dacc3
Rework check inherents to make sure none are missing or extra
Oct 10, 2023
7c04760
actually implement is_inherent methods
Oct 10, 2023
d1afb63
update some old comments
Oct 10, 2023
ca8fdbe
Annotate transactions with original hash identities
Oct 10, 2023
48a9a15
toml sort
Oct 10, 2023
fc1bef3
enforce inherents at beginning of the bloock
Oct 11, 2023
0bca1a7
cleanup some way-too-verbose debugging
Oct 11, 2023
68ca36c
Merge branch 'main' into timestamp
Oct 11, 2023
78e08aa
Cleanup and fix some old TODOs
Oct 11, 2023
b2820ea
cargo fmt
Oct 11, 2023
3bac6b6
bring back transaction priority import
Oct 11, 2023
732bd29
`is_inherent` should not have a default impl in the internal inherent…
Oct 11, 2023
6e3e073
cleanup logging target
Oct 11, 2023
b05377f
Simplify inherent traits and bounds considerably
Oct 11, 2023
2dcc393
Make simple constraint checker actually simple (no inherents allowed)…
Oct 11, 2023
9316d88
Delete a bunch of accidentally included amoeba tests
Oct 11, 2023
004bff8
cargo fmt
Oct 11, 2023
47eacf6
revert unnecessary trait derivation
Oct 11, 2023
3833626
fully qualified path in macro
Oct 11, 2023
bbeb545
revert unrelated change
Oct 11, 2023
6df89a6
restore pool checks for inherents
Oct 11, 2023
f4b2469
Cleanup comments in timestamp tuxedo piece
Oct 11, 2023
3168eb2
don't be so heavy handed with the service
Oct 11, 2023
08204fe
cargo fmt
Oct 11, 2023
33b208e
prune stray file
Oct 11, 2023
88b7b85
comments in macro
Oct 11, 2023
6686fa4
cleanup wrapping and unwrapping in macros
Oct 11, 2023
87435da
followup to simple constraint checkers cannot be inherents.
Oct 11, 2023
6c34da7
prune more too-verbose logging
Oct 11, 2023
3d6a337
simplify trait bounds on transform helper
Oct 11, 2023
7a2257b
relax trait bound on verifier
Oct 11, 2023
3891357
brush up constraint checker
Oct 11, 2023
d8220ab
revise inherents file
Oct 11, 2023
31986e2
fmt
Oct 11, 2023
f25cf1b
clippy
Oct 11, 2023
c2bd33a
unfuck piece tests after some botched search and replace
Oct 12, 2023
80aaa80
Merge branch 'main' into timestamp
Oct 12, 2023
51570b6
Update is_signed tests for clarified semantics
Oct 12, 2023
ebde077
Update TestConstraintChecker to reflect inherents
Oct 12, 2023
ce0f348
Tests for inherent ordering
Oct 12, 2023
c2026d0
Oops, fix logic error. Tests FTW.
Oct 12, 2023
aa27f8f
switch a stray log! to debug!
Oct 12, 2023
96c6987
Fix extrinsics root that changed because the TestTransaction now has …
Oct 12, 2023
6569eba
Better expect messages in service
Oct 12, 2023
057cdb7
better expect messages in the executive.
Oct 12, 2023
7377d96
Better examples for when to use full `VonstraintChecker` trait.
Oct 12, 2023
4f1376e
Merge branch 'main' into timestamp
Oct 12, 2023
81a57a3
Move constants to config trait
Oct 12, 2023
2df4be6
remove old comment
Oct 12, 2023
f3c88ec
Attempt to refactor to single timestamp type.
Oct 12, 2023
1021312
fixed update_timestamp_tests
muraca Oct 13, 2023
3805b52
fixed cleanup_tests and first_block_special_case_tests
muraca Oct 13, 2023
6409334
fixed some comments and typos
muraca Oct 13, 2023
8c09593
remove unnecessary scale_info skip_type_params
Oct 13, 2023
730028a
remove kind of off-topic comment
Oct 13, 2023
8e7350e
add comment from review suggestion
Oct 13, 2023
e0d629d
remove extra space
Oct 13, 2023
d60a841
fix comment typo
Oct 13, 2023
52b2f85
revert ugly consequences of `use super::*;`
muraca Oct 13, 2023
87ee4dc
simplify inherent order checking logic
Oct 13, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@ members = [
"wardrobe/amoeba",
"wardrobe/money",
"wardrobe/poe",
"wardrobe/timestamp",
"wardrobe/kitties",
"wardrobe/runtime_upgrade",
]
resolver = "2"

[workspace.dependencies]
# Generic dependencies
async-trait = "0.1.73"
clap = "4.3.0"
hex-literal = "0.4.1"
jsonrpsee = "0.16.2"
Expand Down
1 change: 1 addition & 0 deletions node/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ sp-blockchain = { workspace = true }

# Local Dependencies
node-template-runtime = { package = "tuxedo-template-runtime", path = "../tuxedo-template-runtime" }
tuxedo-core = { path = "../tuxedo-core" }

[[bin]]
name = "node-template"
Expand Down
32 changes: 22 additions & 10 deletions node/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ pub fn new_full(config: Configuration) -> Result<TaskManager, ServiceError> {
);

let slot_duration = sc_consensus_aura::slot_duration(&*client)?;
let client_for_cidp = client.clone();

let aura = sc_consensus_aura::start_aura::<AuraPair, _, _, _, _, _, _, _, _, _, _>(
StartAuraParams {
Expand All @@ -242,16 +243,27 @@ pub fn new_full(config: Configuration) -> Result<TaskManager, ServiceError> {
select_chain,
block_import,
proposer_factory,
create_inherent_data_providers: move |_, ()| async move {
let timestamp = sp_timestamp::InherentDataProvider::from_system_time();

let slot =
sp_consensus_aura::inherents::InherentDataProvider::from_timestamp_and_slot_duration(
*timestamp,
slot_duration,
);

Ok((slot, timestamp))
create_inherent_data_providers: move |parent_hash, ()| {
let parent_block = client_for_cidp
.clone()
.block(parent_hash)
.expect("a")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like this double expect at all. This method returns a Result, so please update this accordingly

Copy link
Contributor Author

@JoshOrndorff JoshOrndorff Oct 12, 2023

Choose a reason for hiding this comment

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

I've improved the panic messages in 6569eba so that is at least a step in the right direction. If this can be improved more I will need some guided help.

This method returns a Result

I don't think that's true. Here's my understanding. Although the types are complex, so correct me if I'm wrong.

This function with the let parent_block = ... line you quoted is a closure that is being passed in to start the Aura worker. That closure does not return a Result. Rather, it returns an async closure that will, itself, eventually return a Result.

https://paritytech.github.io/polkadot-sdk/master/sp_inherents/trait.CreateInherentDataProviders.html

Copy link
Collaborator

Choose a reason for hiding this comment

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

It took a while, but I found where this is used.
https://github.com/paritytech/polkadot-sdk/blob/7aace06b3d653529ab992c8ff96571d54bf00a36/substrate/client/consensus/slots/src/slots.rs#L154
It logs the error and tries again next slot, so it might not like a panic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely we will not be able to author a block if the database does not know about the previous block. What do you suggest to improve this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was trying earlier to make this return an error when needed, instead of panicking. I wanted to do someting like:

let parent_block = client_for_cidp
    .clone()
    .block(parent_hash)?
    .ok_or(sp_blockchain::Error::UnknownBlock(parent_hash))?
    .block;

There was some issues with the closure being async, and I'm not familiar with that, so please try to make this work without spending too much time on it, otherwise it's fine to leave it like this.

.expect("b")
.block;

async move {
let parent_idp =
tuxedo_core::inherents::ParentBlockInherentDataProvider(parent_block);
let timestamp = sp_timestamp::InherentDataProvider::from_system_time();

let slot =
sp_consensus_aura::inherents::InherentDataProvider::from_timestamp_and_slot_duration(
*timestamp,
slot_duration,
);

Ok((slot, parent_idp, timestamp))
}
},
force_authoring,
backoff_authoring_blocks,
Expand Down
4 changes: 4 additions & 0 deletions tuxedo-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ repository = "https://github.com/Off-Narrative-Labs/Tuxedo"
version = "1.0.0-dev"

[dependencies]
async-trait = { optional = true, workspace = true }
log = { workspace = true }
parity-scale-codec = { features = [ "derive" ], workspace = true }
parity-util-mem = { optional = true, workspace = true }
Expand All @@ -19,6 +20,7 @@ derive-no-bound = { path = "no_bound" }
sp-api = { default_features = false, workspace = true }
sp-core = { default_features = false, workspace = true }
sp-debug-derive = { features = [ "force-debug" ], default_features = false, workspace = true }
sp-inherents = { default_features = false, workspace = true }
sp-io = { features = [ "with-tracing" ], default_features = false, workspace = true }
sp-runtime = { default_features = false, workspace = true }
sp-std = { default_features = false, workspace = true }
Expand All @@ -30,12 +32,14 @@ array-bytes = { workspace = true }
[features]
default = [ "std" ]
std = [
"async-trait",
"sp-debug-derive/std",
"parity-scale-codec/std",
"sp-core/std",
"sp-std/std",
"serde",
"sp-api/std",
"sp-inherents/std",
"sp-io/std",
"sp-runtime/std",
"parity-util-mem",
Expand Down
124 changes: 120 additions & 4 deletions tuxedo-core/aggregator/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,20 +32,35 @@ pub fn aggregate(_: TokenStream, body: TokenStream) -> TokenStream {
)
});
let variants = variant_type_pairs.clone().map(|(v, _t)| v);
let variants2 = variants.clone();
let inner_types = variant_type_pairs.map(|(_v, t)| t);
let inner_types2 = inner_types.clone();

let output = quote! {
// First keep the original code in tact
#original_code

// Now write all the From impls
// Now write all the wrapping From impls
#(
impl From<#inner_types> for #outer_type {
fn from(b: #inner_types) -> Self {
Self::#variants(b)
}
}
)*

// Finally write all the un-wrapping From impls
#(
impl From<#outer_type> for #inner_types2 {
fn from(a: #outer_type) -> Self {
if let #outer_type::#variants2(b) = a {
b
} else {
panic!("wrong type or something...")
}
}
}
)*
};

output.into()
Expand Down Expand Up @@ -115,12 +130,28 @@ pub fn tuxedo_constraint_checker(attrs: TokenStream, body: TokenStream) -> Token
let variants = variant_type_pairs.clone().map(|(v, _t)| v);
let inner_types = variant_type_pairs.map(|(_v, t)| t);

let vis = ast.vis;
// Set up the names of the new associated types.
let mut error_type_name = outer_type.to_string();
error_type_name.push_str("Error");
let error_type = Ident::new(&error_type_name, outer_type.span());
let inner_types = inner_types.clone();

let mut inherent_hooks_name = outer_type.to_string();
inherent_hooks_name.push_str("InherentHooks");
let inherent_hooks = Ident::new(&inherent_hooks_name, outer_type.span());

let vis = ast.vis;

// TODO there must be a better way to do this, right?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link

@gabriele-0201 gabriele-0201 Oct 15, 2023

Choose a reason for hiding this comment

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

The first problem here is that the value can't be cloned in the quote macro (as far as I know, if it would be possible with some magic trick then the solution would be obvious) so all the clones need to be done before the macro, and that repeated code is the result. An easy solution is to involve a macro_rules to avoid repeat all the syntax but the inner problem is that inside macro_rules identifiers can't be created, then I came up with two solutions:

  1. the first one need to provide to the macro the name of the variables and the name of the variable that needs to be copied, but this result into more lines and even uglier then the repeated code
  2. Using the paste macro, that enables you to append identifiers to create new ones, the code is the following:
    macro multiple_assign {
        ($($num:literal),* = $var:ident) => {
            paste!{ $(let [<$var $num>] = $var.clone();)* }
        }
    }
    
    multiple_assign!(2,3,4,6 = inner_types);
    multiple_assign!(2,3,4,5,6 = variants);

the final code is not so smaller than the previous one but is more scalable if the required copies of inner_types and variants will grow in the future

JoshOrndorff marked this conversation as resolved.
Show resolved Hide resolved
let inner_types2 = inner_types.clone();
let inner_types3 = inner_types.clone();
let inner_types4 = inner_types.clone();
let inner_types6 = inner_types.clone();
let variants2 = variants.clone();
let variants3 = variants.clone();
let variants4 = variants.clone();
let variants5 = variants.clone();
let variants6 = variants.clone();

let output = quote! {
// Preserve the original enum, and write the From impls
#[tuxedo_core::aggregate]
Expand All @@ -138,9 +169,84 @@ pub fn tuxedo_constraint_checker(attrs: TokenStream, body: TokenStream) -> Token
)*
}

/// This type is generated by the `#[tuxedo_constraint_checker]` macro.
/// It is a combined set of inherent hooks for the inherent hooks of each individual checker.
///
/// This type is accessible downstream as `<OuterConstraintChecker as ConstraintChecker>::InherentHooks`
#[derive(Debug, scale_info::TypeInfo)]
#vis enum #inherent_hooks {
#(
#variants2(<#inner_types2 as tuxedo_core::ConstraintChecker<#verifier>>::InherentHooks),
)*
}

impl tuxedo_core::inherents::InherentInternal<#verifier, #outer_type> for #inherent_hooks {

fn create_inherents(
authoring_inherent_data: &InherentData,
previous_inherents: Vec<(tuxedo_core::types::Transaction<#verifier, #outer_type>, sp_core::H256)>,
) -> Vec<tuxedo_core::types::Transaction<#verifier, #outer_type>> {

let mut all_inherents = Vec::new();

#(
{
// Filter the previous inherents down to just the ones that came from this piece
let previous_inherents = previous_inherents
.iter()
.filter_map(|(tx, hash)| {
match tx.checker {
#outer_type::#variants3(ref inner_checker) => Some((tx.transform::<#inner_types3>(), *hash )),
_ => None,
}
})
.collect();

let inherents = <#inner_types3 as tuxedo_core::ConstraintChecker<#verifier>>::InherentHooks::create_inherents(authoring_inherent_data, previous_inherents)
.iter()
.map(|tx| tx.transform::<#outer_type>())
.collect::<Vec<_>>();
all_inherents.extend(inherents);
}
)*

// Return the aggregate of all inherent extrinsics from all constituent constraint checkers.
all_inherents
}

fn check_inherents(
importing_inherent_data: &sp_inherents::InherentData,
inherents: Vec<tuxedo_core::types::Transaction<#verifier, #outer_type>>,
result: &mut sp_inherents::CheckInherentsResult,
) {
#(
let relevant_inherents: Vec<tuxedo_core::types::Transaction<#verifier, #inner_types4>> = inherents
.iter()
.filter_map(|tx| {
match tx.checker {
#outer_type::#variants4(ref inner_checker) => Some(tx.transform::<#inner_types4>()),
_ => None,
}
})
.collect();

<#inner_types4 as tuxedo_core::ConstraintChecker<#verifier>>::InherentHooks::check_inherents(importing_inherent_data, relevant_inherents, result);

// According to https://paritytech.github.io/polkadot-sdk/master/sp_inherents/struct.CheckInherentsResult.html
// "When a fatal error occurs, all other errors are removed and the implementation needs to abort checking inherents."
if result.fatal_error() {
return;
}
)*
}

}

impl tuxedo_core::ConstraintChecker<#verifier> for #outer_type {
type Error = #error_type;

type InherentHooks = #inherent_hooks;

fn check (
&self,
inputs: &[tuxedo_core::types::Output<#verifier>],
Expand All @@ -149,10 +255,20 @@ pub fn tuxedo_constraint_checker(attrs: TokenStream, body: TokenStream) -> Token
) -> Result<TransactionPriority, Self::Error> {
match self {
#(
Self::#variants2(inner) => inner.check(inputs, peeks, outputs).map_err(|e| Self::Error::#variants2(e)),
Self::#variants5(inner) => inner.check(inputs, peeks, outputs).map_err(|e| Self::Error::#variants5(e)),
)*
}
}

fn is_inherent(&self) -> bool {
match self {
#(
Self::#variants6(inner) => <#inner_types6 as tuxedo_core::ConstraintChecker<#verifier>>::is_inherent(inner),
)*
}

}

}
};

Expand Down
Loading
Loading