Accumulators for intra-block book-keeping #105

JoshOrndorff · 2023-09-29T21:35:56Z

Overview

This PR adds a feature that is called "accumulators" in the code and I've also considered calling "scratchpads". An accumulator serves as a temporary place for Tuxedo pieces to do some accounting within a single block. The accumulator will be cleared automatically by the Tuxedo executive at the end of each block, so the stored data will not be available in the next block, or even after the block's execution closes.

Usecases

Tallying up the tips from each transaction so that the block author can claim them at the end of each block.
Making sure that inherents are set exactly once in each block.
No other concrete ones yet, but I suspect there will be.

Fit in Substrate

This feature is similar to paritytech/polkadot-sdk#359 in some ways. However it is implemented in the Tuxedo core which is entirely in the runtime. That issue asks for host functions and performance which this will not provide. This also does not satisfy @burdges's request to have the data in-memory and skip scale encoding.

Drawbacks

This feature is pretty un-UTXO-y in the sense that we are basically providing (a strict structured interface around) storage items to the Tuxedo pieces. I've tried very hard to avoid storage items. I even went to great lengths to handle inherents in a UTXO-native way in #100 and I'm glad I did.

The thing that makes me mostly okay with this is that the storage is cleared at the end of each block, so the data does not persist. I think the term "scratchpad" emphasizes this well as it is a temporary place to write some notes while doing a job and it is not part of the final project.

I have not yet thought of another way to achieve giving tips to block authors, a feature that has existed even since bitcoin. Indeed, even in bitcoin there is a dedicated variable for tallying such tips.

Refactor and Features

@clangenb I remember you saying you like it when the refactor goes in first and the feature that motivated comes in another PR. That has stuck with me. Once I get this ironed out, I plan to separate the core feature from the way I'm using it in the money piece, and put them in separately. Thanks for inspiring more discipline.

The overall reason is that production chains will want to spend tokens but not mint. Of course the immediate motivation for me is that I only spending will need the scratchpad, and I want the code to be more isolated.

burdges · 2023-09-29T22:10:25Z

As an aside, I'd think "pallets" could work like this:

use spin::Mutex; // https://docs.rs/spin/latest/spin/index.html

struct Scratch { ... }

impl {
    const fn new() -> Scratch { ... }
    fn finalize(self) -> Result { ... }
}

static MUTEX: Mutex<Option<PalletAcc>> = Mutex::new(None);

fn on_initialize() -> Result {
    let lock = MUTEX.try_lock().expect("Reentrancy bug!");
    *lock = Some(Scratch::new())
}

fn verify_tx() -> Result {
    // Setup validation
    let lock = MUTEX.try_lock().expect("Reentrancy bug!");
    match lock.deref_mut() {
        None => {
            // we're not inside execute_block so do full tx validation, abort if fails
        },
        Some(scratch) => { 
            // we're inside execute_block so use scratch: &mut Scratch
        }.
    }
    drop(lock);
    // Finish validation, abort if fails
}

fn on_finalize() -> Result {
    let lock = MUTEX.try_lock().expect("Reentrancy bug!");
    let scratch = lock.deref_mut().take()
    drop(lock);
    scratch.finalize()
}

We do not require the spin lock per se, since the runtime is single threaded, but whatever.

tuxedo-core/src/constraint_checker.rs

JoshOrndorff · 2023-10-08T23:42:28Z

Oh crap, I haven't actually updated to executive to use the new accumulator logic. So there is still that part. But anyway this is coming together now, and some review on the design is very welcome.

- `key_path` now returns `[u8]` instead of `str` - added and used various `From` implementations - implemented `ConstraintCheckingSuccess::transform` - fixed various tests - ran rustfmt Signed-off-by: muraca <[email protected]>

muraca · 2023-10-09T12:43:14Z

@JoshOrndorff I hope this commit is helpful.
The From/Into implementation for ConstraintCheckingSuccess was not feasible, so I added a new method that does exactly that, please use that one instead of the ugly conversion in the proc macro

coax1d

Looks good but lots of comment clean ups with regards to Todos and some other questions I had

coax1d · 2023-10-09T13:54:16Z

tuxedo-core/aggregator/src/lib.rs

-                        Self::#variants2(inner) => inner.check(inputs, peeks, outputs).map_err(|e| Self::Error::#variants2(e)),
+                        Self::#variants6(inner) => inner.check(inputs, peeks, outputs)
+                        .map(|old| {
+                            //TODO I would really rather have an into or from impl for ConstraintCheckingSuccess, but that's just won't compile


Is this still a necessary TODO comment?

Yes, I was stuck/confused on this last night. Matteo's transform method is a good way forward.

tuxedo-core/src/constraint_checker.rs

tuxedo-core/src/types.rs

coax1d · 2023-10-09T14:12:05Z

tuxedo-core/src/types.rs

+    pub provides: Vec<Vec<u8>>,
+    pub requires: Vec<Vec<u8>>,


any better description for what these Vec<Vec<u8>> are? An alias of any kind? if not is what it is.

wardrobe/money/src/lib.rs

coax1d · 2023-10-09T14:17:17Z

tuxedo-core/src/constraint_checker.rs

+    /// This is a function and takes a value, as opposed to being a constant for an important reason.
+    /// Aggregate runtimes made from multiple pieces will need to give a different initial value depending
+    /// which of the constituent constraint checkers is being called.
+    fn initial_value(_: Self::ValueType) -> Self::ValueType;


Will this just always be the Default for the ValueType associated type? Should ValueType just instead implement the Default trait?

I thought about that same thing. All the examples of accumulators that I thought of can indeed be implemented using the default value as the initial value.

I ended up going with a separate method for these reasons:

Semantics. While it seems the initial value of the accumulator will typically be the default, there is no fundamental reason it must be this way. Someone may write an unintuitive impl of Default for that purpose.

To mirror https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.fold

The main one: For aggregate runtimes. When the constraint checker is composed with the macro from individual constraint checkers, it needs to do an actual match and return a different initial value depending on which piece is being called. Default does allow taking a param and returning a different result based on that param.

JoshOrndorff · 2023-10-09T15:56:13Z

tuxedo-core/src/types.rs

+impl<A> From<ValidTransaction> for PreliminarilyValidTransaction<A> {
+    fn from(transaction: ValidTransaction) -> Self {
+        Self {
+            provides: transaction.provides,
+            requires: transaction.requires,
+            priority: transaction.priority,
+            intermediate_accumulator_value: None,
+        }
+    }
+}


@muraca What is the point of this one? When would we need to go in this reverse direction from valid to preliminarily valid?

I'll have to double check tomorrow, but I think I did this because it wasn't possible to use some of the ValidTransactionBuilder's fields

wardrobe/money/src/lib.rs

JoshOrndorff · 2023-10-09T16:01:38Z

tuxedo-core/aggregator/src/lib.rs

    let inner_types = inner_types.clone();
+    let inner_types2 = inner_types.clone();
+    let inner_types3 = inner_types.clone();
+    let inner_types4 = inner_types.clone();
+    let inner_types5 = inner_types.clone();
    let variants2 = variants.clone();
+    let variants3 = variants.clone();
+    let variants4 = variants.clone();
+    let variants5 = variants.clone();
+    let variants6 = variants.clone();


There's gotta be a better way to do this right? Does anyone know? I'm pretty much a macro newb. This crate is by far the most complex one I've ever written.

Co-authored-by: Andrew <[email protected]>

JoshOrndorff · 2023-10-09T16:44:14Z

Thanks for the reviews everyone. I know this PR is pretty far along, but I thought of an alternative design for tallying up fees and giving them to the block author and I need to write it out so we can consider them. My biggest complaint about the accumulators was always that they were not very UTXO native and were more like storage items, and this alternative design solves that problem.

When users send transactions, they must ensure that the input coins exceed the value of the output coins. They also have the ability to protect those output coins with whatever verifiers they want. Typically it might require a signature from the intended recipient. Of course token senders can also specify outputs that are UpForGrabs and can be claimed by anyone. And this is the key to the alternate design.

When you send tokens, you explicitly specify a tip to the block author by putting UpForGrabs. Only tokens that are explicitly included in an output as UpForGrabs may be claimed by the block author. If the input value exceeds the output value, then the difference truly is burned.

Then block authors claim their reward by inserting a transaction at the end of the block that sweeps up all of those UpForGrabs coins that were created during the block. There is no risk that someone else will get to those coins first because the block author can simply drop any transactions that try to claim the UpForGrabs ones.

The implementation would roughly be that when authoring, the block author notes all the UpForGrabs coins in a temporary storage that is cleared before the block ends just like how we do the extrinsics now.

The key benefits are:

All the tip-related state is still in UTXOs, not in the accumulator.
Importing nodes don't need to know anything about the implementation of this feature. They only have to check the normal UTXO rules.

JoshOrndorff · 2023-10-13T16:22:50Z

Okay, I'm going to close this one for now.

Block rewards can be done more elegantly by the alternate design described above, and enforcing the right number of inherent calls can be done in the offchain pre-checks.

If we ever decide we need it, we have the design done and can re-open this.

Joshy Orndorff added 9 commits September 29, 2023 14:56

Split money constraint checker into two constraint checkers.

98bd53c

The overall reason is that production chains will want to spend tokens but not mint. Of course the immediate motivation for me is that I only spending will need the scratchpad, and I want the code to be more isolated.

Sketch partial idea for accumulator style scratch pads.

7388c20

attempt to adapt money piece for accumulator.

46b8200

stub accumulator in other pieces.

7bad45a

derive some traits

c6aa543

little fixes in money piece

28a5db6

visibility

e0f1025

cargo fmt

fd78188

fix unit signature

94259bd

This was referenced Oct 2, 2023

Timestamp Inherent #100

Merged

End of Block Inherents (to tip block authors) #108

Open

Joshy Orndorff added 9 commits October 8, 2023 17:42

Slightly revise accumulator trait and add lots of docs

104f99a

warnings

df1a824

first attempt at aggregate accumulators

8bec4e7

Getting closer I think

0484e3c

restructure initial value and some other stuff

7c621c0

fix some warnings

3599bbb

cargo fmt

9c76399

workaround missing into again

9644ebf

proper error for invalid params to outer accumulator.

52f0c77

JoshOrndorff marked this pull request as ready for review October 8, 2023 23:32

JoshOrndorff requested review from coax1d and muraca October 8, 2023 23:32

JoshOrndorff commented Oct 8, 2023

View reviewed changes

tuxedo-core/src/constraint_checker.rs Outdated Show resolved Hide resolved

Joshy Orndorff and others added 4 commits October 8, 2023 20:12

Sketch out changes required for the executive.

4f761ac

Sketch code instead of todos, didn't even check if it compiles yet.

98acf96

Couple cleanups. More to go, but I think this will work.

c74e922

lots of improvements:

42f475c

- `key_path` now returns `[u8]` instead of `str` - added and used various `From` implementations - implemented `ConstraintCheckingSuccess::transform` - fixed various tests - ran rustfmt Signed-off-by: muraca <[email protected]>

coax1d requested changes Oct 9, 2023

View reviewed changes

JoshOrndorff commented Oct 9, 2023

View reviewed changes

wardrobe/money/src/lib.rs Outdated Show resolved Hide resolved

JoshOrndorff commented Oct 9, 2023

View reviewed changes

JoshOrndorff and others added 4 commits October 9, 2023 12:19

Typos and comment cleanups from code review

bdb81f8

Co-authored-by: Andrew <[email protected]>

Prune outdated todo

a323669

cleanup comments around priority saturation

fa797b5

spelling imbalance

3de2e96

JoshOrndorff mentioned this pull request Oct 9, 2023

End of block inherents #114

Closed

JoshOrndorff closed this Oct 13, 2023

JoshOrndorff mentioned this pull request Nov 13, 2023

Options for how to store the relay parent number #138

Closed

burdges mentioned this pull request Dec 6, 2023

Custom child trie paritytech/polkadot-sdk#2605

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accumulators for intra-block book-keeping #105

Accumulators for intra-block book-keeping #105

JoshOrndorff commented Sep 29, 2023 •

edited

Loading

burdges commented Sep 29, 2023 •

edited

Loading

JoshOrndorff commented Oct 8, 2023

muraca commented Oct 9, 2023

coax1d left a comment

coax1d Oct 9, 2023

JoshOrndorff Oct 9, 2023

coax1d Oct 9, 2023

coax1d Oct 9, 2023

JoshOrndorff Oct 9, 2023 •

edited

Loading

JoshOrndorff Oct 9, 2023

muraca Oct 9, 2023

JoshOrndorff Oct 9, 2023

JoshOrndorff commented Oct 9, 2023

JoshOrndorff commented Oct 13, 2023

Accumulators for intra-block book-keeping #105

Accumulators for intra-block book-keeping #105

Conversation

JoshOrndorff commented Sep 29, 2023 • edited Loading

Overview

Usecases

Fit in Substrate

Drawbacks

Refactor and Features

burdges commented Sep 29, 2023 • edited Loading

JoshOrndorff commented Oct 8, 2023

muraca commented Oct 9, 2023

coax1d left a comment

Choose a reason for hiding this comment

coax1d Oct 9, 2023

Choose a reason for hiding this comment

JoshOrndorff Oct 9, 2023

Choose a reason for hiding this comment

coax1d Oct 9, 2023

Choose a reason for hiding this comment

coax1d Oct 9, 2023

Choose a reason for hiding this comment

JoshOrndorff Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

JoshOrndorff Oct 9, 2023

Choose a reason for hiding this comment

muraca Oct 9, 2023

Choose a reason for hiding this comment

JoshOrndorff Oct 9, 2023

Choose a reason for hiding this comment

JoshOrndorff commented Oct 9, 2023

JoshOrndorff commented Oct 13, 2023

JoshOrndorff commented Sep 29, 2023 •

edited

Loading

burdges commented Sep 29, 2023 •

edited

Loading

JoshOrndorff Oct 9, 2023 •

edited

Loading