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

feat: implement the dash analogue to BIP143 #5860

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

panleone
Copy link

@panleone panleone commented Feb 3, 2024

Issue being fixed or feature implemented

Implement the bip143 analogue for dash, see here for documenation (dashpay/dips#149).

What was done?

A new sigHashType SIGHASH_DIP0143 has been added and txs inputs with this new sigHashType are signed with the new signature hash algorithm specified in the document.
Feature is activated with an EHF hard fork and controlled with the script flag SCRIPT_ENABLE_DIP0143

How Has This Been Tested?

Many tests have been added:

  • Verify that the new signature hash is generated correctly for a set of 100 transactions.
  • Verify that script verification will accept new signatures only if there script flag SCRIPT_ENABLE_DIP0143 is set, and the signed input contains the sigHashType SIGHASH_DIP0143
  • Verify that both signature (with new and old algorithm) can coexist in the same multisig transaction input and pass script verification, provided that the script flag SCRIPT_ENABLE_DIP0143 is set.
  • Verify that the feature is rejected before activation.

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@panleone panleone marked this pull request as draft February 3, 2024 11:56
@knst knst added this to the 21 milestone Feb 4, 2024
Copy link

github-actions bot commented Feb 6, 2024

This pull request has conflicts, please rebase.

@UdjinM6
Copy link

UdjinM6 commented Feb 7, 2024

Would be nice to have 7be17ee in a separate PR. Could maybe also introduce and use another helper like

bool HasExtraPayloadField() const
{
    return IsSpecialTxVersion() && nType != TRANSACTION_NORMAL;
}

while at it.

@UdjinM6
Copy link

UdjinM6 commented Feb 12, 2024

pls check https://github.com/UdjinM6/dash/commits/pr5860/ branch for wip suggestions, specifically 84905a2

NOTE: I had to revert 0ef6ded and 160dc04 to make it work and also cf150bd to avoid merge conflicts in the future but the latter is optional I guess

@panleone
Copy link
Author

pls check https://github.com/UdjinM6/dash/commits/pr5860/ branch for wip suggestions, specifically 84905a2

NOTE: I had to revert 0ef6ded and 160dc04 to make it work and also cf150bd to avoid merge conflicts in the future but the latter is optional I guess

My thoughts:

...
// Drop the signatures when SIGHASH_DIP0143 is not used, since there's no way for a signature to sign itself
    int nHashType = vchSig.empty() ? 0 : vchSig.back();
    if (nHashType & SIGHASH_DIP0143) {
        // Can't use using SIGHASH_DIP0143 hash type without SCRIPT_ENABLE_DIP0143 flag
        if ((flags & SCRIPT_ENABLE_DIP0143) == 0) {
            return set_error(serror, SCRIPT_ERR_SIG_FINDANDDELETE); //TODO
        }
    } else {
        int found = FindAndDelete(scriptCode, CScript() << vchSig);
        if (found > 0 && (flags & SCRIPT_VERIFY_CONST_SCRIPTCODE))
            return set_error(serror, SCRIPT_ERR_SIG_FINDANDDELETE);
    }
...

and

...
// With SCRIPT_VERIFY_CONST_SCRIPTCODE, OP_CODESEPARATOR is rejected even in an unexecuted branch
if (opcode == OP_CODESEPARATOR && /*sigversion == SigVersion::BASE && TODO*/ (flags & SCRIPT_VERIFY_CONST_SCRIPTCODE))
...

DIP0143 would have been a nice moment for imposing no OP_CODESEPARATOR and no signatures inside the script, but we already have a flag for that, SCRIPT_VERIFY_CONST_SCRIPTCODE. I think it is already enough and we don't need to re-impose those conditions with DIP0143, we would just overcomplicate it imo (or there is a particular reason for those additions that I am missing?).

@panleone
Copy link
Author

pls check https://github.com/UdjinM6/dash/commits/pr5860/ branch for wip suggestions, specifically 84905a2

NOTE: I had to revert 0ef6ded and 160dc04 to make it work and also cf150bd to avoid merge conflicts in the future but the latter is optional I guess

My idea was that with the current implementation that we have DIP143 can be the very simple: when the right sighash_type is set serialize with the new algorithm , without changing anything else

@UdjinM6
Copy link

UdjinM6 commented Feb 12, 2024

ok, so then smth like that 1eb9b4e on top I guess?

@panleone
Copy link
Author

ok, so then smth like that 1eb9b4e on top I guess?

So that the overall effect is moving this check from CheckSig

if ((nHashType & SIGHASH_DIP0143) && sigversion != SigVersion::DIP0143){
    return false;
}

to EvalScript?

// Can't use using SIGHASH_DIP0143 hash type without SCRIPT_ENABLE_DIP0143 flag
if ((nHashType & SIGHASH_DIP0143) && (~flags & SCRIPT_ENABLE_DIP0143)) {
    return set_error(serror, SCRIPT_ERR_SIG_FINDANDDELETE); //TODO: introduce and use new script error

yes, I like it so we can even get rid of GetSigVersionFromFlags and probably of the whole enum SigVersion

@UdjinM6
Copy link

UdjinM6 commented Feb 12, 2024

....
yes, I like it so we can even get rid of GetSigVersionFromFlags and probably of the whole enum SigVersion

not sure we can do that but let's see - we need tests :)

@panleone panleone force-pushed the dip_143 branch 2 times, most recently from e8e4762 to 774991a Compare February 23, 2024 10:02
Only if this flag is enabled it's possible to use the new sighash algorithm
Since Sigversion is now a local property of each input.
- Test that txs with SigVersion DIP0143 pass script verification only when the corresponding flag is set
- Test that multi sig transactions can have their inputs signed with different SigVersion (one BASE, the other DIP0143)
- Test that SigHashType DIP0143 cannot be used alone
So it is possible to generate DIP0143 SigHash from python tests
@panleone panleone marked this pull request as ready for review February 25, 2024 08:59
@panleone
Copy link
Author

....
yes, I like it so we can even get rid of GetSigVersionFromFlags and probably of the whole enum SigVersion

not sure we can do that but let's see - we need tests :)

At the end I tried both methods and the one withSigVersion was much better, so I did not remove it.
So I reverted almost all refactor commits and the only left are:
437ef2f, which however can be removed to have less diff lines;
abad7f4, which removes SigVersion from EvalScript (because that variable was unused)

@UdjinM6
Copy link

UdjinM6 commented Feb 26, 2024

Looks good so far! Pls see b100c66, d69d345 and 429db53 for some suggestions/fixes.

@panleone
Copy link
Author

Looks good so far! Pls see b100c66, d69d345 and 429db53 for some suggestions/fixes.

review applied, with the addition of 00d60b9

PastaPastaPasta added a commit that referenced this pull request Apr 2, 2024
8b6c96d refactor: a new constant with Tx Version (Alessandro Rezzi)
9d429f4 refactor: drop functions from struct which supposed to be simple as possible (Alessandro Rezzi)
b2bb097 refactor: simplify vExtra using in wallet and add const (Alessandro Rezzi)
d9f0e93 refactor: use CTransaction for immutable tx in evo_assetlocks_tests (Alessandro Rezzi)
ca0fe8c refactor: introduce HasExtraPayloadField() (Alessandro Rezzi)
72d2008 refactor: introduce IsSpecialTxVersion() (Alessandro Rezzi)

Pull request description:

  ## Issue being fixed or feature implemented
  Refactor extracted from #5860. Even if that PR should not need anymore this commit (since it has been found a better way to activate dip143), I think It's still a worthy refactor

  ## What was done?
    Introduce  `tx.IsSpecialTxVersion()` in place of `tx.nVersion == 3`,  `tx.nVersion >= 3`
    and `tx.HasExtraPayloadField()` in place of  `tx.nVersion == 3 && tx.nType != TRANSACTION_NORMAL`

  ## How Has This Been Tested?

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [X] I have performed a self-review of my own code
  - [X] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  knst:
    utACK 8b6c96d

Tree-SHA512: aa16f9ee570e0fa86561cc440ffba40f6d554caa3e08b630b481732b899cc613eef596c672b5a20dbf3582ad109ffb687f4ad815f712dc16f636f8857d98480a
PastaPastaPasta added a commit that referenced this pull request Jun 3, 2024
238978e fix: adjust `signrawtransactionwithkey` help text (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  `amount` was introduced in #3101. Double checked the code and yes, we do pass it around (for compatibility reasons) but it doesn’t affect the sig right now, you can set it to 0 or just skip it completely so it should be `optional`, not `required`. We even have a test that uses `signrawtransactionwithkey ` and ignores `amount` https://github.com/dashpay/dash/blob/master/test/functional/rpc_signrawtransaction.py#L19-L46.

  NOTE: It might become required for `sighashtype` with `SIGHASH_DIP0143` flag after #5860 activation.

  kudos to @pshenmic for noticing

  ## What was done?
  Adjust help text

  ## How Has This Been Tested?
  Run tests

  ## Breaking Changes
  n/a

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

Top commit has no ACKs.

Tree-SHA512: e028c46b8b9c1663d9940642b08d56444ab4e5ab33015af1cb99265338b75f9e1c156cbbdd8e00f313bce87117019c769241cc4d83ccd6693ec0ffbaa8940e89
@UdjinM6 UdjinM6 removed this from the 21 milestone Jul 26, 2024
Copy link

This pull request has conflicts, please rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants