Softfork infrastructure and coinid operator #273

arvidn · 2023-04-21T10:45:04Z

There are two parts to this PR, best reviewed seperately;

the infrastructure to support the softfork operator.
- Moving the softfork logic into the interpreter, to allow giving it apply-semantics, and the unit tests are moved from test_ops into run_program (since the former doesn't run the interpreter)
- Adding a stack of softfork guard frames, used to record the expected cost to check against when exiting a guard. When exiting a softfortk guard, the allocator is also reset to the state of when entering it, to reclaim memory
- Parsing and checking its arguments, preserving the existing relaxed rules in consensus mode.
- validating the cost argument (when execution completes)
- The cost of the softfork operator is 140, slightly higher than apply
Add the coinid operator as part of softfork extension 0
- Having a valid extension value enables testing the validation of the cost argument (so, more tests are added in this PR)

richardkiss · 2023-04-25T17:13:54Z

src/more_ops.rs

+    }
+    let args = args.rest()?;
+    let amount = atom(args.first()?, "coinid")?;
+    if !amount.is_empty() {


Can you use uint_atom::<8> here?

yes, I will make this change

actually, in this case I want to preserve the buffer, to pass into the Sha256 context, not actually parse the number. I just want to ensure the constraints are met. I could use uint_atom() just to check the constraints, and ignore its return value though, but then there would be wasted work in producing the integer

You might consider checking the assembly generated... it's possible the optimizer might realize you don't use the return value. Also, since it likely just goes into a register anyway, it's probably not particularly expensive.

(I may be being a bit naïvely optimistic about how good the optimizers are.)

I looked into splitting the validation and the parsing of the integer and realized that another important difference is that coinid requires the amount to be in canonical integer form. i.e. it does not allow any redundant leading zero. This is an important rule since it affects the hash.

The only shared check is really that the value is not negative. There are some utility functions to sanitize integers in chia_rs, for parsing conditions. Perhaps in the future (especially as we have more operators in here) it might make sense to move those into this repo and have some stricter conventions for validating arguments. I don't think there are enough places we do this in right now though.

Even your suggestion of calling uint_atom::<8>() just for the validation won't work, because redundant leading zeros would still have to be checked.

richardkiss · 2023-04-25T17:14:36Z

src/run_program.rs

+// this represents the state we were in before entering a soft-fork guard. We
+// may need this to long-jump out of the guard, and also to validate the cost
+// when exiting the guard
+struct SoftforkGuard {


Do we still need this when there is no long-jump capabilities?

yes, 2 main reasons:

we need to record the expected cost that was specified when we entered the softfork guard. Once we exit, we need to compare against it. Now, this expected cost could be a field in the Softfork operation that checks this, but that would bloat that type from 1 byte to (probably) 16 bytes.

keep track of which extension is currently enabled.

resetting the allocator is the current 3rd use, but that's not strictly necessary. If we would use a Dialect object instead of an extension enum, this is where that object would go, as I understand it.

richardkiss · 2023-04-25T17:18:12Z

src/dialect.rs

@@ -2,10 +2,26 @@ use crate::allocator::{Allocator, NodePtr};
 use crate::cost::Cost;
 use crate::reduction::Response;

+#[repr(u32)]
+#[derive(Clone, Copy, Eq, PartialEq)]
+pub enum Extension {


I'm trying to understand why we need this new structure. I'd always figured the Dialect object would be the thing that changes based on extension. So you have the base dialect, which is as we see, then a dialect level 0 which adds coinid and the bls12 ops. Is there something that makes Extension necessary? Isn't it just sort of an add-on to the Dialect?

it is specifying which operators should be available. It's simple and efficient and it restricts extensions to just add new operators, not change whether we're in mempool mode, the heap or stack limits or the opcodes for apply, quote or softfork.

Having this enum saves us from having multiple dialect objects (with a lot of repeated code, or code shared via inheritance)

in a way Dialect might be a pretty good name for this enum, but it's taken

richardkiss · 2023-04-25T17:19:27Z

src/run_program.rs

@@ -66,6 +88,7 @@ struct RunProgramContext<'a, D> {
    val_stack: Vec<NodePtr>,
    env_stack: Vec<NodePtr>,
    op_stack: Vec<Operation>,
+    softfork_stack: Vec<SoftforkGuard>,


As per my comment above, why not just use a stack of dialects, and at the end of the softfork eval process, pop off the top dialect.

we would also need to record the expected cost in those stack frames. It would also need dynamic dispatch for the operators, unless of course this stack is of Dialect objects of the same type. In that case, the only state held by the dialect object would be the Extension field, for it to know which operators are supposed to be available. But that would avoid the dynamic dispatch. It would also make the existing RuntimeDialect quite expensive, because its (immutable) state would be duplicated in each stack frame.

richardkiss · 2023-04-25T21:32:59Z

src/chia_dialect.rs

@@ -21,6 +21,10 @@ pub const LIMIT_HEAP: u32 = 0x0004;
 // When set, enforce a stack size limit for CLVM programs
 pub const LIMIT_STACK: u32 = 0x0008;

+// When set, we allow softfork with extension 0 (which includes coinid and the
+// BLS operators)
+pub const ENABLE_BLS_OPS: u32 = 0x0010;


Ah, I see, as we move through the blockchain consensus, we have to adjust how the softfork operator works.

I feel like this can be completely encapsulated in Dialect. For simplicity, suppose a soft fork kicks in that introduces a new dialect that supports coinid.

You would still need some way for the full node to communicate to the rust implementation extensions are available. Before a soft fork activates, the interpreter must behave as if the extension doesn't exist.

Now, in my patch, whether an extension is available or not is controlled by the dialect, see line 137 below. This flag is used to communicate whether the softfork has activated or not.

would you construct a new ChiaDialect object for every softfork operator?
Presumably you wouldn't start a new interpreter, so you would perhaps have a stack of dialect objects, and execute operators using the top dialect of the stack. The two main issues I see is that it makes the dialect object (and operator dispatch) dynamic rather than static (which is likely to be costly). It's not obvious that having separate classes for the separate dialects really buys us anything, since (presumably) they would be mostly the same.

richardkiss · 2023-04-26T22:30:37Z

src/chia_dialect.rs

@@ -56,12 +60,15 @@ impl Dialect for ChiaDialect {
        o: NodePtr,
        argument_list: NodePtr,
        max_cost: Cost,
+        extensions: Extension,


Suggested change

extensions: Extension,

extension: Extension,

richardkiss · 2023-04-26T22:32:44Z

src/chia_dialect.rs

+        &[36]
+    }
+
+    fn softfork_extension(&self, ext: u32) -> Extension {


What about passing in the Extension directly instead of using flags?

richardkiss · 2023-04-26T22:47:35Z

src/run_program.rs

+            // differently depending on whether we allow unknown ops or not
+            let (ext, prg, env) = match self.parse_softfork_arguments(&operand_list) {
+                Ok(ret_values) => ret_values,
+                Err(err) => {


Is this a potentially temporary workaround here?

richardkiss · 2023-04-26T23:03:45Z

src/run_program.rs

+                    self.apply_op(cost, effective_max_cost - cost),
+                    max_cost_ptr,
+                )?,
+                Operation::Softfork => self.exit_softfork_guard(cost)?,


The names are a bit inconsistent here. Maybe rename the operator to match the name of the function it calls? exit_guard & ExitGuard, or EndSoftfork and end_soft_fork, or whatever.

richardkiss · 2023-04-26T23:06:24Z

src/test_ops.rs

@@ -878,6 +856,27 @@ pubkey_for_exp 0x73eda753299d7d483339d80809a1d80553bda402fffe5bfeffffffff00f0000
 pubkey_for_exp 0x8c1258acd66282b7ccc627f7f65e27faac425bfd0001a40100000000fffffffe => 0xb7f1d3a73197d7942695638c4fa9ac0fc3688c4f9774b905a14e3a3f171bac586c55e83ff97a1aeffb3af00adb22c6bb | 1327426
 pubkey_for_exp 0x8c1258acd66282b7ccc627f7f65e27faac425bfd0001a40100000000ffffffff => 0xc00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000 | 1327426
 pubkey_for_exp 0x8c1258acd66282b7ccc627f7f65e27faac425bfd0001a4010000000000000000 => 0x847f5fcce0b9aa0f2bb3de6847337c9ed1bc2184a125c232721e1c81b0f0fee78506790a78c98abff2dd4b01a0756352 | 1327426
+
+; BLS extensions


Suggested change

; BLS extensions

; BLS extensions could go here

richardkiss

I think Extension is a bit vague and could do with a different name. To me, it seems like it represents the maximum soft fork version number known to this current runtime. Maybe AdditionalOpcodeSet or something like that?

richardkiss · 2023-04-27T22:51:10Z

src/run_program.rs

+    allocator_state: Checkpoint,
+
+    // this specifies which new operators are available
+    extension: Operators,


Consider renaming to operators.

richardkiss

Seems good enough for now. If you want to keep working on it, that's fine too. Let me know if you need further approvals.

One comment I want to make is that coinid probably isn't really that useful of a thing to be behind the softfork guard... probably repurposing an unused operator is a better way to do it (although it would have to be assert_coinid). It'd take the three fields along with a hash, and it'd assert the field sizes and that the hash is correct. Since coinid is stuck behind the guard, either the whole puzzle has to be there or the hash has to be passed in so it can confirm it's right, effectively making it assert_coinid. We should probably discuss if there's any use to adding this operator as a guarded softfork op. For now, it's a decent test case.

arvidn force-pushed the softfork branch 2 times, most recently from 65253be to 7f5df6c Compare April 21, 2023 23:56

arvidn added 2 commits April 24, 2023 20:41

implement infrastructure for softfork extensions

3a245a6

add new coinid operator under softfork guard (extension 0)

95fe193

arvidn force-pushed the softfork branch from 7f5df6c to 95fe193 Compare April 24, 2023 18:41

arvidn marked this pull request as ready for review April 24, 2023 18:41

arvidn changed the title ~~Softfork infrastructure~~ Softfork infrastructure and coinid operator Apr 25, 2023

arvidn requested a review from richardkiss April 25, 2023 06:46

richardkiss reviewed Apr 25, 2023

View reviewed changes

richardkiss reviewed Apr 26, 2023

View reviewed changes

richardkiss reviewed Apr 27, 2023

View reviewed changes

fixup

67b5520

richardkiss reviewed Apr 27, 2023

View reviewed changes

richardkiss approved these changes Apr 27, 2023

View reviewed changes

arvidn merged commit 2dc450e into main Apr 28, 2023

arvidn deleted the softfork branch April 28, 2023 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Softfork infrastructure and coinid operator #273

Softfork infrastructure and coinid operator #273

arvidn commented Apr 21, 2023 •

edited

Loading

richardkiss Apr 25, 2023

arvidn Apr 25, 2023

arvidn Apr 26, 2023

richardkiss Apr 26, 2023

arvidn Apr 27, 2023

richardkiss Apr 25, 2023

arvidn Apr 25, 2023

richardkiss Apr 25, 2023

arvidn Apr 25, 2023

arvidn Apr 27, 2023

richardkiss Apr 25, 2023

arvidn Apr 25, 2023

richardkiss Apr 25, 2023

arvidn Apr 25, 2023

richardkiss Apr 26, 2023

richardkiss Apr 26, 2023

richardkiss Apr 26, 2023

richardkiss Apr 26, 2023

richardkiss Apr 26, 2023

richardkiss left a comment

richardkiss Apr 27, 2023

richardkiss left a comment

Softfork infrastructure and coinid operator #273

Softfork infrastructure and coinid operator #273

Conversation

arvidn commented Apr 21, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardkiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardkiss left a comment

Choose a reason for hiding this comment

arvidn commented Apr 21, 2023 •

edited

Loading