Backport changes in TX verification/signing API #3101
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a preparation for BIP143 support.
Includes simplifications by Eric Lombrozo.
This was referenced Sep 20, 2019
Merged
PastaPastaPasta
requested changes
Sep 20, 2019
nmarley
reviewed
Sep 22, 2019
| @@ -605,7 +605,8 @@ bool CPrivateSendServer::IsInputScriptSigValid(const CTxIn& txin) | |||
| if (nTxInIndex >= 0) { //might have to do this one input at a time? | |||
| txNew.vin[nTxInIndex].scriptSig = txin.scriptSig; | |||
| LogPrint(BCLog::PRIVATESEND, "CPrivateSendServer::IsInputScriptSigValid -- verifying scriptSig %s\n", ScriptToAsmStr(txin.scriptSig).substr(0, 24)); | |||
| if (!VerifyScript(txNew.vin[nTxInIndex].scriptSig, sigPubKey, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_STRICTENC, MutableTransactionSignatureChecker(&txNew, nTxInIndex))) { | |||
| // TODO we're using amount=0 here but we should use the correct amount. This works because Dash ignores the amount while signing/verifying (only used in Bitcoin/Segwit) | |||
There was a problem hiding this comment.
Does this just mean calculating the amount from the txIns above and adding that here?
We won't actually ever use this even then, right?
There was a problem hiding this comment.
Never say never :D Signing the input amount could be very useful (e.g. for HW wallets https://blog.trezor.io/what-segregated-witness-means-for-trezor-808c790a05bd), imo we should consider adding this feature one day.
As for TODO part - we could just use txNew.vout[0].nValue here because we know all inputs and outputs MUST be of the same value, this is verified earlier and we should not get here if this is not the case.
There was a problem hiding this comment.
Yepp agree with @UdjinM6 about never saying never ;)
The "good" thing here is, if we ever start to use the amount while signing, it will break PS code and then this TODO comment will give the hint on what needs to be fixed :D
nmarley
approved these changes
Sep 22, 2019
5 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This backports 3 commits from bitcoin#8149, which is the initial implementation of SegWit. We completely skipped this PR as we do not want SegWit in Dash. However, the PR also included changes to the TX verification and signing API, which later PRs depend on. To avoid a mess in backporting future PRs, we should backport the API changes before.
Due to the API changes, we now introduce
SigVersionas well, which is used in Bitcoin to indicate normal TXs vs. SegWit TXs. In Dash, we only haveSIGVERSION_BASEto keep code level compatibility.Also, we track the TX input amount now but then never actually use it when signing TXs. The amount is only used in Bitcoin SegWit TXs. We still keep tracking it for code level compatibility.