multi: replace ExtractPkScriptAddrs consensus calls. #2036
Conversation
dnldd
changed the title
dnldd
force-pushed
the
replace_extractpkscriptaddrs_consensus
branch
from
8308e64 to
aef9927
Compare
dnldd
force-pushed
the
replace_extractpkscriptaddrs_consensus
branch
from
aef9927 to
c3b00ea
Compare
There was a problem hiding this comment.
I started to review this and left a comment on the first instance, but this does not at all seem to be correct. It's changing a bunch of calls and adding a bunch of stuff that is not in any way involved in consensus.
I should specifically note that there should absolutely not be any notion of ExtractPkScriptAdds in the stake package. That is, nearly by definition, dealing with standardness since addresses don't really exist at all and are just an abstraction on top of standardized scripts.
| @@ -8,8 +8,12 @@ github.com/dchest/siphash v1.2.1 h1:4cLinnzVJDKxTCl9B01807Yiy+W7ZzVHj/KIroQRvT4= | |||
| github.com/dchest/siphash v1.2.1/go.mod h1:q+IRvb2gOSrUnYoPqHiyHXS0FOBBOdl6tONBlVnOnt4= | |||
| github.com/decred/base58 v1.0.2 h1:yupIH6bg+q7KYfBk7oUv3xFjKGb5Ypm4+v/61X4keGY= | |||
| github.com/decred/base58 v1.0.2/go.mod h1:pXP9cXCfM2sFLb2viz2FNIdeMWmZDBKG3ZBYbiSM78E= | |||
| github.com/decred/dcrd/chaincfg v1.5.2 h1:dd6l9rqcpxg2GF5neBmE2XxRc5Lqda45fWmN4XOJRW8= | |||
There was a problem hiding this comment.
This doesn't seem right. Nothing should be depending on an old chaincfg at this point.
| @@ -240,10 +240,10 @@ func (idx *ExistsAddrIndex) ConnectBlock(dbTx database.Tx, block, parent *dcruti | |||
| // is used for the script version. This will ultimately need to | |||
| // updated to support new script versions. | |||
| const scriptVersion = 0 | |||
| if txscript.IsMultisigSigScript(txIn.SignatureScript) { | |||
| rs := txscript.MultisigRedeemScriptFromScriptSig( | |||
| if stake.IsMultisigSigScript(scriptVersion, txIn.SignatureScript) { | |||
There was a problem hiding this comment.
The exists address index is not consensus and therefore nothing in this file should be updated.
|
I think a little more elaboration is probably necessary. As you probably know, for a transaction to be valid, there are a bunch of rules it must follow. One of those rules is that the scripts have to execute and end up with an "Pay-to-X" scripts and, by extension, the addresses that represent them, are merely an abstraction on top to make it easier for humans to transact. They are standardized (aka standard) scripts. As stated above though, consensus does not care about that. Unfortunately, the stake system additions conflated that a bit and made a specific subset of standard scripts actually required by consensus for those specific type of transactions. Namely, stake transactions are required to use scripts of the So, the idea isn't to just move the extraction code, it's to avoid it altogether. The consensus rules need to be very explicit about the forms that are required instead of calling functions meant to identify standard scripts and piggybacking on them. |
|
Confirmed with @davecgh there are no more consensus sites to update for |
This localizes
txscript.ExtractPkScriptAddrsto theblockchainpackage to handle consensus calls.Resolves #1625