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

Implement a script execution engine #1291

Merged
merged 6 commits into from
Nov 16, 2020

Conversation

AndrejMitrovic
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic commented Oct 28, 2020

I've loosely based it on the Bitcoin execution engine, adding code attribution for any borrowed code.

Unlike Bitcoin, we do not have to support historic opcodes such as P2SH (see isPayToScriptHash), but may instead replace it with something smaller by introducing a locking script tag.

Missing features:

  • Time locks (todo later)
  • SIGHASH support (needed for Eltoo as well as advanced scripting)
  • Integer arithmetic. We may want to add OP_ADD, etc, however inadvertently this brings in platform-specific behavior and needs lots of care. E.g. signed vs unsigned.
  • Integration into Agora

There will be more work put into this, but I'll likely do it in another branch.

Part of: #237

@AndrejMitrovic AndrejMitrovic added the type-feature An addition to the system introducing new functionalities label Oct 28, 2020
@AndrejMitrovic AndrejMitrovic added this to the 3. Flash Layer milestone Oct 28, 2020
@AndrejMitrovic AndrejMitrovic linked an issue Oct 28, 2020 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Oct 28, 2020

Codecov Report

Merging #1291 (1205582) into v0.x.x (3a70998) will increase coverage by 1.71%.
The diff coverage is 98.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##           v0.x.x    #1291      +/-   ##
==========================================
+ Coverage   78.31%   80.03%   +1.71%     
==========================================
  Files         108      116       +8     
  Lines        9026     9892     +866     
==========================================
+ Hits         7069     7917     +848     
- Misses       1957     1975      +18     
Flag Coverage Δ
integration 37.82% <0.00%> (-2.73%) ⬇️
unittests 78.59% <98.01%> (+1.89%) ⬆️

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

Impacted Files Coverage Δ
source/agora/script/Engine.d 96.98% <96.98%> (ø)
source/agora/script/Script.d 99.02% <99.02%> (ø)
source/agora/script/Lock.d 100.00% <100.00%> (ø)
source/agora/script/Opcodes.d 100.00% <100.00%> (ø)
source/agora/script/ScopeCondition.d 100.00% <100.00%> (ø)
source/agora/script/Stack.d 100.00% <100.00%> (ø)
source/agora/network/NetworkClient.d 87.75% <0.00%> (-6.13%) ⬇️
source/agora/consensus/data/DataPayload.d 98.21% <0.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a70998...1205582. Read the comment docs.

/// without having to encode it in a separate length byte. 64 was chosen as
/// the upper bound because our signatures are 64 bytes.
/// For pushes of data longer than 64 bytes use the `PUSH_DATA_*` opcodes.
enum OP : ubyte
Copy link
Member

@TrustHenry TrustHenry Oct 28, 2020

Choose a reason for hiding this comment

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

Great! a completely unique new OPcode definition.

@bpalaggi bpalaggi assigned bpalaggi and AndrejMitrovic and unassigned bpalaggi Oct 29, 2020
@AndrejMitrovic AndrejMitrovic removed their assignment Oct 29, 2020
@AndrejMitrovic
Copy link
Contributor Author

I'll take care of the code coverage soon. It seems I missed some test-cases, though 92.60% is pretty good. :P

Copy link
Contributor Author

@AndrejMitrovic AndrejMitrovic left a comment

Choose a reason for hiding this comment

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

Some comments.

source/agora/script/Engine.d Outdated Show resolved Hide resolved
source/agora/script/Engine.d Outdated Show resolved Hide resolved
source/agora/script/Engine.d Outdated Show resolved Hide resolved
source/agora/script/Engine.d Outdated Show resolved Hide resolved
source/agora/script/Engine.d Outdated Show resolved Hide resolved
source/agora/script/Lock.d Outdated Show resolved Hide resolved
source/agora/script/ScopeCondition.d Outdated Show resolved Hide resolved
source/agora/script/ScopeCondition.d Show resolved Hide resolved
source/agora/script/ScopeCondition.d Show resolved Hide resolved
source/agora/script/ScopeCondition.d Show resolved Hide resolved
@AndrejMitrovic
Copy link
Contributor Author

AndrejMitrovic commented Oct 30, 2020

I've fixed all the comments.

I now have to increase the code coverage.

@AndrejMitrovic AndrejMitrovic force-pushed the exec-engine branch 3 times, most recently from 1f6b9ba to 525240e Compare October 30, 2020 12:52
@AndrejMitrovic
Copy link
Contributor Author

I've increased code coverage to 100%

(minus the assert(0); which is unreachable in some places)

@AndrejMitrovic
Copy link
Contributor Author

AndrejMitrovic commented Oct 30, 2020

I've originally thought that I don't have to add support for a multisig opcode because we have Schnorr, but I've realized that our signature scheme is insecure for regular users. I think for now I'll skip adding the opcode, but we'll have to think of a good solution to the following problem.

It's secure for the Validators between them to use them because they already committed and signed with their private key (through the enrollment), proving they own the key. But it's not secure for users because of the possibility of rogue key attacks. MuSig is one key aggregation scheme which tries to solve it. The following is a good read (I've highlighted the relevant part):

https://blockstream.com/2018/01/23/en-musig-key-aggregation-schnorr-signatures/#:~:text=fuss

We currently don't have anyone working on Schnorr. But this will become important soon.

For users to initiate channels and therefore use the flash layer we need to have a secure multi-party signature scheme for the funding transactions.

It doesn't block us from any work right now. We can build an implementation now, and harden the security later. I'll open up a Schnorr-related issue in the coming days.

@AndrejMitrovic
Copy link
Contributor Author

For anyone who is interested in MuSig (and also other schemes), I highly recommend this guide: https://tlu.tarilabs.com/cryptography/musig-schnorr-sig-scheme/The_MuSig_Schnorr_Signature_Scheme.html

There are other chapters in this book which are great to study.

source/agora/script/Lock.d Outdated Show resolved Hide resolved
source/agora/script/Engine.d Outdated Show resolved Hide resolved
@AndrejMitrovic AndrejMitrovic force-pushed the exec-engine branch 3 times, most recently from e537f9f to c44ed0a Compare November 10, 2020 02:43
@AndrejMitrovic
Copy link
Contributor Author

Pushed a change to add the Input parameter to execute(). It's needed to implement SigHash.NoInput and other sighash types for the signatures. It's folded into the previous commit.

@AndrejMitrovic
Copy link
Contributor Author

Removed isValidPointBytes() as it's now part of Point.isValid() in a recent master change. And also fixed the accidental unittest breakage.

AndrejMitrovic added a commit to AndrejMitrovic/agora that referenced this pull request Nov 10, 2020
@AndrejMitrovic
Copy link
Contributor Author

Update: Added OP.VERIFY_SIG because it doesn't make sense to split it into a separate PR. This change also introduced a verifySignature method which is used by both CHECK_SIG and VERIFY_SIG. It will become even more useful once SigHash is added.

@AndrejMitrovic
Copy link
Contributor Author

I will merge this later today if there are no further comments.

This PR does not integrate the engine with Agora yet, it's part of another PR.

@AndrejMitrovic
Copy link
Contributor Author

This went through a lengthy self-review and received no other comments in the ~20 days it's been open. I'll merge and continue working on the other integration & feature PRs.

The scripting language supports a number of
built-in opcodes, which may later be extended.

Each opcode is one byte. The opcodes include:
- Stack pushes, from 1 up to 65535 bytes (ushort).
- Conditional opcodes such as IF / NOT_IF / ELSE
- CHECK_* opcodes which push their result to the stack
- VERIFY_* opcodes which fail the script execution
  when the result of the operation is false
- Hashing, stack item duplication opcodes
- and more..
It uses a linked-list rather than a vector to avoid unnecessary copying
due to stomping prevention as the same item may be popped and later
pushed
to the stack. In addition, this makes it very cheap to copy the stack as
all internal items are immutable anyway.

The stack must be initialized with a set of size constraints,
the maximum size of the stack, and the maximum size of any one item
on the stack.
It contains syntax validation as well as some
helper routines.

The `validateScript` is the entry point which can
be used to create a syntactically validated Script
out of the provided byte array of opcodes.

If the byte array contain any unknown opcodes,
the function will return an error string.

Lock scripts may contain any of the supported
opcodes, whereas unlocks scripts may only
consist of stack push opcodes. This is for
security reasons - otherwise an unlock script
could potentially cause premature successfull
script evaluation without satisfying the
constraints of the lock script.

The module also contains some unittest helper
routines, to help with creating various
lock / unlock script types.
These are the types that will ultimately
replace the `signature` in the `Input`,
and the `address` in the `Output`.

The Lock type contains a tag, allowing 4
different types of lock scripts:

- Key => lock is a 64-byte public key,
         unlock is expected to be a signature.
- KeyHash => lock is a hash of a 64-byte public key,
             unlock is expected to be a pair of
             <signature, public-key>.
             This form may be used for better privacy.
- Script => lock is a script that will be evaluated
            by the engine. The unlock script may either
            be empty or only contain stack push opcodes.
- Redeem => lock is a <redeem-hash>, and unlock may
            only contain stack push opcodes, where the
            last push will be read as the redeem script.
It's used for tracking which conditional scopes are true where
the code may be executed, or not true where code execution will be
skipped until we exit into a scope which is true.

Largely based on Bitcoin's ConditionStack implementation,
with simplifications and a series of tests added.

MIT license attribution was added.
The engine supports 4 different lock scripts:
    - Key => lock is a 64-byte public key,
             unlock is expected to be a signature.
    - KeyHash => lock is a hash of a 64-byte public key,
                 unlock is expected to be a pair of
                 <signature, public-key>.
                 This form may be used for better privacy.
    - Script => lock is a script that will be evaluated
                by the engine. The unlock script may either
                be empty or only contain stack push opcodes.
    - Redeem => lock is a <redeem-hash>, and unlock may
                only contain stack push opcodes, where the
                last push will be read as the redeem script.

In addition it supports arbitrary nesting of IF / ELSE
conditionals, and the set of opcodes as defined in
the `Opcodes.d` module.

Because the script lock contains a tag, it is possible
to extend and plug in a webassembly engine at a later point.

This commit does not integrate the Engine into Agora.
This should be part of a future commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature An addition to the system introducing new functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Basic script execution engine
3 participants