Implement Schnorr Checksig / CheckDatasig Modifications to the Scripting System #134
Conversation
…minor DER Signature Encoding name change
|
The reason for CI failures are due to the remaining Segwit Recovery test failures I will close this and recommit it. So it's more organized @tuxcanfly could I ask for pointers involving the segwit recovery function found at: |
|
@alwaysAn0n check this out |
lib/script/script.js
Outdated
| @@ -3112,6 +3134,11 @@ class Script { | |||
| const raw = stack.pop(); | |||
| const redeem = Script.fromRaw(raw); | |||
|
|
|||
| if (!(flags & Script.flags.VERIFY_SEGWIT_RECOVERY) !== 0 | |||
There was a problem hiding this comment.
you need to check for the flag to be set, the ! is doing the opposite. Also the reason for some failing tests.
There was a problem hiding this comment.
You're the man , thanks getting rid of that symbol corrects all the failing tests for Segwit Recovery.
|
Hey guy's im not trying to make things confusing here, but if you fork the software with my schnorr-branch and run all the script-tests with bmocha they all pass. Im not sure as to why ci is throwing assert.ifError on 30 tests with Schnorr. |
|
@tuxcanfly or @nodar-chkuaselidze can you guys check the Travis CI script is correct? |
lib/script/script.js
Outdated
| @@ -1131,7 +1131,7 @@ class Script { | |||
| || !(sig[sig.length - 1] & Script.hashType.SIGHASH_FORKID)) | |||
| subscript.findAndDelete(sig); | |||
|
|
|||
| validateTXSignature(sig, flags); | |||
| CheckTransactionSignature(sig, flags); | |||
There was a problem hiding this comment.
Function names are generally camel cased with the first letter being lowercase in this codebase. There are multiple functions that are camel cased that start with uppercased letters
|
Please be sure to fix the linting errors and its important to make the tests pass in CI. Have you tried syncing the node against the upgraded testnet? |
|
I’ll fix it now |
looks like its failing on |
Looks like there are some remaining lint issues: |
|
@tuxcanfly Tests passed locally for me after switching to |
As it happens, when I switched to So it looks like the CI is fetching the dependencies from a previous build which probably has no schnorr package built. As to why it's not saving the cache and restoring it, I have no clue 🤷♂️ |
|
Hmm, it could be that |
|
It seems that current version of the -- This causes bcrypto to fall back to You can reproduce: -- -- -- UPDATE Update: both problems should have been fixed in bcrypto. |
|
The reason why build fails after |
Codecov Report
@@ Coverage Diff @@
## master #134 +/- ##
==========================================
- Coverage 55.55% 55.51% -0.05%
==========================================
Files 109 109
Lines 27184 27238 +54
Branches 4498 4513 +15
==========================================
+ Hits 15102 15121 +19
- Misses 12082 12117 +35
Continue to review full report at Codecov.
|
|
How's this going? Anything I can do to help? |
|
@andrewcottage Thanks for being interested in helping out Andrew! More review can always help and also verifying that it can sync on the Schnorr Testnet. Also verifying that all of the tests from Bitcoin ABC were ported over properly |
|
Utilize this branch if you're going to sync the lastest testnet in the readme explains what to do, there's a bcash.conf file that you would like to add in your .bcash directory where the chain is going to stored locally. |
|
This is going to be my last day in adding the rest of the Stack Tests, for regular schnorr transactions / segwit recovery. Feel free to ask any questions |
|
Sounds good. I'm having an internet outage, so I won't be able to sync the chain tonight (tethering to my phone currently). I might be able to do that tomorrow along with comparing tests. |
|
I keep getting When trying to sync the chain. I tried both main net and testnet |
|
This is due to running the tx / addr indexers I’ll be porting over the changes , bug fixes made to bcoin in my branch in the next day |
|
|
||
| if (await tx.verifyAsync(view, flags, this.workers)) { | ||
| throw new VerifyError(tx, | ||
| 'nonstandard', | ||
| 'non-mandatory-script-verify-flag', | ||
| 0); | ||
| } | ||
|
|
||
| if (await tx.verifyAsync(view, |
There was a problem hiding this comment.
In principle this stuff can be take out now that the upgrade has happened.
|
Overall it looks like the code is doing the right things related to the new activated features, however I don't know javascript well so it's hard to say clearly. The fact that this synced to the fork testnet and now the main chain is a actually good sign, given that the new features are quite minimal changes. I have noticed on discussions that there were issues with bcash, but they appear to be unrelated to these changes. |
Implemented Schnorr Signature Checks on the following Opcodes:
OP_CHECKSIG / OP_CHECKDATSIG.
Along with MTP Activation for the Hardfork found in the chain.js file (perhaps these commits shouldn't have been added in conjunction with these particular commits).
In which case, I'd be more then willing to close this and re-commit the PR with the Segwit Recovery changes in a separate branch as well.
I will be adding extra primitives to the script-test.js file found in the Unit-Test directory. Along with Segwit Recovery checks to the verify functions in the script.js file.