Remove redundant code in MutateTxSign(CMutableTransaction&, const std::string&) #10694

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:remove-redundant-code-in-MutateTxSign changing 1 files +0 −10

practicalswift commented at 6:02 PM on June 28, 2017: contributor

Remove redundant code in MutateTxSign(CMutableTransaction&, const std::string&).
fanquake added the label Refactoring on Jun 29, 2017
practicalswift force-pushed on Aug 9, 2017
laanwj commented at 10:38 AM on August 23, 2017: member

So do we really not care if the resulting signed transaction is complete or not? (if we do, this a bug instead of redundant code)
laanwj added the label Utils and libraries on Aug 23, 2017
practicalswift commented at 12:28 PM on August 23, 2017: contributor

@laanwj Yes, I agree it looks odd :-)
practicalswift commented at 10:52 AM on January 28, 2018: contributor

What should we do about this one? Shall I close? :-)
MarcoFalke commented at 5:12 PM on January 28, 2018: member

Needs rebase due to travis bug
Remove redundant code in MutateTxSign(CMutableTransaction&, const std::string&) b1149ee4c3
practicalswift force-pushed on Jan 29, 2018
practicalswift commented at 8:10 AM on January 29, 2018: contributor

@MarcoFalke Thanks! Now rebased! :-)
practicalswift commented at 4:09 PM on February 8, 2018: contributor

What shall we do about this one? Leaving dead code around feels icky :-)
MarcoFalke commented at 3:37 AM on February 12, 2018: member

utACK b1149ee4c3e4b7301017f779a6dec4415bcb733d
jonasschnelli added the label Scripts and tools on Feb 12, 2018
jonasschnelli removed the label Utils/log/libs on Feb 12, 2018
jonasschnelli added the label Utils/log/libs on Feb 12, 2018
jonasschnelli removed the label Scripts and tools on Feb 12, 2018
laanwj commented at 9:02 AM on February 12, 2018: member

What shall we do about this one? Leaving dead code around feels icky :-)

No one really answered my question. Do we care about this incompleteness? Should it be in the output? I never use bitcoin-tx myself so I can't answer it.
jonasschnelli commented at 9:15 AM on February 12, 2018: contributor

@practicalswift: care to elaborate why this is dead code? I haven't check all possible states, but for me it looks like that VerifyScript() after combining signatures and updated the transaction makes pretty sense... but maybe I miss something.
practicalswift force-pushed on Feb 12, 2018
practicalswift commented at 10:01 AM on February 12, 2018: contributor

Pushed an updated version which only removes trivial dead code. Now keeping the VerifyScript(…) call and its side-effects (if any).

Thanks for reviewing @jonasschnelli

Please re-review :-)
jonasschnelli commented at 10:08 AM on February 12, 2018: contributor

This seems to make sense now. utACK 347fd7ca1742183707541ef186f97218aab6e0a9
practicalswift force-pushed on Mar 11, 2018
practicalswift commented at 10:02 PM on March 11, 2018: contributor

@jonasschnelli Would you mind re-reviewing? Had to force push to get Travis to rebuild :-) No changes since 347fd7c.

in src/bitcoin-tx.cpp:655 in 48b93d9b2d outdated

 658 | -    }
 659 | -
 660 | -    if (fComplete) {
 661 | -        // do nothing... for now
 662 | -        // perhaps store this for later optional JSON output
 663 | +        VerifyScript(txin.scriptSig, prevPubKey, &txin.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, MutableTransactionSignatureChecker(&mergedTx, i, amount));

MarcoFalke commented at 10:18 PM on March 11, 2018:

The return code is ignored and you are only passing const references. Mind to elaborate on the side effects this has?

practicalswift commented at 10:26 PM on March 12, 2018: contributor

Updated version. Please re-review :-) @jonasschnelli - did I misunderstand you regarding the call to VerifyScript()? As @MarcoFalke commented there seems to be no side-effects from it and the return value is not used so no need to keep it around? What am I missing? :-)
MarcoFalke commented at 11:59 PM on March 12, 2018: member

The current commit is now identical to the original commit (b1149ee). I'd prefer you force push b1149ee, so I don't have to re-ACK. ;)
practicalswift force-pushed on Mar 13, 2018
practicalswift commented at 7:22 AM on March 13, 2018: contributor

@MarcoFalke Fixed :-)
promag commented at 9:17 AM on March 13, 2018: member

utACK b1149ee4.
laanwj merged this on Mar 13, 2018
laanwj closed this on Mar 13, 2018
laanwj referenced this in commit ae5bcc7abb on Mar 13, 2018
PastaPastaPasta referenced this in commit e1eda31281 on Dec 12, 2020
PastaPastaPasta referenced this in commit 1cb26e84ce on Dec 12, 2020
PastaPastaPasta referenced this in commit 0637728d55 on Dec 15, 2020
PastaPastaPasta referenced this in commit 3448da7802 on Dec 15, 2020
PastaPastaPasta referenced this in commit 03200fcb66 on Dec 18, 2020
practicalswift deleted the branch on Apr 10, 2021
gades referenced this in commit 62ee8315f2 on Apr 22, 2022
DrahtBot locked this on Aug 18, 2022

Contributors

Labels

Refactoring Utils/log/libs