[Refactor] Combine scriptPubKey and amount as CTxOut in CScriptCheck #10953

jl2012 · 2017-07-30T09:40:53Z

This simplifies CScriptCheck by combining scriptPubKey and amount

sipa · 2017-07-30T09:44:00Z

Concept ACK

NicolasDorier · 2017-07-30T13:35:03Z

VerifyScript shouldn't have TxOut as well instead of 2 different params ?

TheBlueMatt · 2017-07-30T14:22:33Z

Concept ACK

jl2012 · 2017-07-30T15:12:13Z

@NicolasDorier seems can't do that because scriptPubKey and nValue go to different places.

Christewart · 2017-07-30T19:02:52Z

utack 3457ef1

sipa · 2017-07-31T00:05:01Z

@NicolasDorier VerifyScript knows nothing about transaction structure.

NicolasDorier · 2017-07-31T00:20:51Z

woops indeed I missed it utACK 3457ef1

promag · 2017-07-31T00:26:54Z

src/validation.h

-        scriptPubKey(scriptPubKeyIn), amount(amountIn),
-        ptxTo(&txToIn), nIn(nInIn), nFlags(nFlagsIn), cacheStore(cacheIn), error(SCRIPT_ERR_UNKNOWN_ERROR), txdata(txdataIn) { }
+    CScriptCheck(): out(CTxOut()), ptxTo(0), nIn(0), nFlags(0), cacheStore(false), error(SCRIPT_ERR_UNKNOWN_ERROR) {}
+    CScriptCheck(const CTxOut& outIn, const CTransaction& txToIn, unsigned int nInIn, unsigned int nFlagsIn, bool cacheIn, PrecomputedTransactionData* txdataIn) :


Remove CTxOut(). Nit, class members are prefixed.

afk11 · 2017-08-06T08:26:33Z

Concept ACK

jl2012 · 2017-08-30T12:39:34Z

rebased

promag · 2017-08-30T22:49:56Z

utACK e912118.

theuni · 2017-08-31T15:59:09Z

utACK e912118

jtimon · 2017-09-06T00:47:00Z

utACK e912118

jnewbery

utACK

jnewbery · 2017-09-18T15:30:23Z

src/validation.h

@@ -355,8 +355,7 @@ bool CheckSequenceLocks(const CTransaction &tx, int flags, LockPoints* lp = null
 class CScriptCheck
 {
 private:
-    CScript scriptPubKey;
-    CAmount amount;
+    CTxOut out;


nit: I know it's tedious, but can you name this according to the new style guide. In this case m_tx_out

Where could I see the new style guide? And what does the "m" mean? Actually I think "prevout" is the most accurate name but the name is already used.

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md

Class member variables have a m_ prefix.

actually the name out is taken from https://github.com/bitcoin/bitcoin/blob/v0.15.0.1/src/coins.h#L33 . So for consistency we have to keep it.

I don't understand. That's for Coin class member. Why does the CScriptCheck member need to be named the same simply because it's of the same type?

jnewbery · 2017-09-18T15:32:04Z

src/validation.h

-        scriptPubKey(scriptPubKeyIn), amount(amountIn),
-        ptxTo(&txToIn), nIn(nInIn), nFlags(nFlagsIn), cacheStore(cacheIn), error(SCRIPT_ERR_UNKNOWN_ERROR), txdata(txdataIn) { }
+    CScriptCheck(): ptxTo(nullptr), nIn(0), nFlags(0), cacheStore(false), error(SCRIPT_ERR_UNKNOWN_ERROR) {}
+    CScriptCheck(const CTxOut& outIn, const CTransaction& txToIn, unsigned int nInIn, unsigned int nFlagsIn, bool cacheIn, PrecomputedTransactionData* txdataIn) :


Style guide says that arguments should be snake case, but I think it's fine name this outIn to match the style of the other arguments in the constructor.

TheBlueMatt · 2017-09-21T01:27:13Z

utACK e912118 (modulo it would be nice to use m_out or m_tx_out as @jnewbery suggested in CScriptCheck).

… CScriptCheck 3a131b7 Rename out to m_tx_out in CScriptCheck (Johnson Lau) e912118 [Refactor] Combine scriptPubKey and amount as CTxOut in CScriptCheck (Johnson Lau) Pull request description: This simplifies CScriptCheck by combining scriptPubKey and amount Tree-SHA512: 6422363cf5394c6cfefb30c1709db6def63230b809cc7697887e4a2e8c684149208edf91dd139e031b9fe732776b2db59305f77c3cba6f333b11cceb39ef0cc2

Revert "Rename out to m_tx_out in CScriptCheck" This reverts commit 3a131b7. -and- Revert Merge #10953: [Refactor] Combine scriptPubKey and amount as CTxOut in CScriptCheck" This reverts commit aeed345, reversing changes made to c6223b3.

…TxOut in CScriptCheck" This reverts commit aeed345, reversing changes made to c6223b3.

…xOut in CScriptCheck 3a131b7 Rename out to m_tx_out in CScriptCheck (Johnson Lau) e912118 [Refactor] Combine scriptPubKey and amount as CTxOut in CScriptCheck (Johnson Lau) Pull request description: This simplifies CScriptCheck by combining scriptPubKey and amount Tree-SHA512: 6422363cf5394c6cfefb30c1709db6def63230b809cc7697887e4a2e8c684149208edf91dd139e031b9fe732776b2db59305f77c3cba6f333b11cceb39ef0cc2

… in CScriptCheck Summary: 3a131b7 Rename out to m_tx_out in CScriptCheck (Johnson Lau) e912118 [Refactor] Combine scriptPubKey and amount as CTxOut in CScriptCheck (Johnson Lau) Pull request description: This simplifies CScriptCheck by combining scriptPubKey and amount --- Backport of Core [[bitcoin/bitcoin#10953 | PR10953]] Test Plan: ninja check && ninja check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6932

…CScriptCheck 2af6794 Rename out to m_tx_out in CScriptCheck (Johnson Lau) 5f6cd00 [Refactor] Combine scriptPubKey and amount as CTxOut in CScriptCheck (Johnson Lau) Pull request description: Straightforward. Coming from bitcoin#10953 > This simplifies CScriptCheck by combining scriptPubKey and amount ACKs for top commit: furszy: ACK 2af6794 Tree-SHA512: dbc2358315d2bbcd6313dfd41eb28c1a364e9b0b88f6b22c4b36ef3784849ccd2239fd035a784846f1ecbf90d5511d908f1df384c351c1d22d86bbb7da5d28ad

fanquake added the Validation label Jul 30, 2017

promag reviewed Jul 31, 2017

View reviewed changes

[Refactor] Combine scriptPubKey and amount as CTxOut in CScriptCheck

e912118

jl2012 force-pushed the combine_script_amount branch from 3457ef1 to e912118 Compare August 30, 2017 10:33

jnewbery reviewed Sep 18, 2017

View reviewed changes

Rename out to m_tx_out in CScriptCheck

3a131b7

sipa merged commit 3a131b7 into bitcoin:master Sep 22, 2017

underdarkskies mentioned this pull request Jul 3, 2018

Extended RPC to support Insight-API RavenProject/Ravencoin#106

Closed

underdarkskies referenced this pull request in underdarkskies/Ravencoin Jul 14, 2018

Revert "Merge #10953: [Refactor] Combine scriptPubKey and amount as C…

900b89f

…TxOut in CScriptCheck" This reverts commit aeed345, reversing changes made to c6223b3.

random-zebra mentioned this pull request May 9, 2021

[Refactor] Combine scriptPubKey and amount as CTxOut in CScriptCheck PIVX-Project/PIVX#2373

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor] Combine scriptPubKey and amount as CTxOut in CScriptCheck #10953

[Refactor] Combine scriptPubKey and amount as CTxOut in CScriptCheck #10953

jl2012 commented Jul 30, 2017 •

edited

sipa commented Jul 30, 2017

NicolasDorier commented Jul 30, 2017

TheBlueMatt commented Jul 30, 2017

jl2012 commented Jul 30, 2017

Christewart commented Jul 30, 2017

sipa commented Jul 31, 2017

NicolasDorier commented Jul 31, 2017

promag Jul 31, 2017

afk11 commented Aug 6, 2017

jl2012 commented Aug 30, 2017

promag commented Aug 30, 2017

theuni commented Aug 31, 2017

jtimon commented Sep 6, 2017

jnewbery left a comment

jnewbery Sep 18, 2017

jl2012 Sep 19, 2017

jnewbery Sep 19, 2017

jl2012 Sep 19, 2017

jnewbery Sep 19, 2017

jnewbery Sep 18, 2017

TheBlueMatt commented Sep 21, 2017

[Refactor] Combine scriptPubKey and amount as CTxOut in CScriptCheck #10953

[Refactor] Combine scriptPubKey and amount as CTxOut in CScriptCheck #10953

Conversation

jl2012 commented Jul 30, 2017 • edited

sipa commented Jul 30, 2017

NicolasDorier commented Jul 30, 2017

TheBlueMatt commented Jul 30, 2017

jl2012 commented Jul 30, 2017

Christewart commented Jul 30, 2017

sipa commented Jul 31, 2017

NicolasDorier commented Jul 31, 2017

promag Jul 31, 2017

Choose a reason for hiding this comment

afk11 commented Aug 6, 2017

jl2012 commented Aug 30, 2017

promag commented Aug 30, 2017

theuni commented Aug 31, 2017

jtimon commented Sep 6, 2017

jnewbery left a comment

Choose a reason for hiding this comment

jnewbery Sep 18, 2017

Choose a reason for hiding this comment

jl2012 Sep 19, 2017

Choose a reason for hiding this comment

jnewbery Sep 19, 2017

Choose a reason for hiding this comment

jl2012 Sep 19, 2017

Choose a reason for hiding this comment

jnewbery Sep 19, 2017

Choose a reason for hiding this comment

jnewbery Sep 18, 2017

Choose a reason for hiding this comment

TheBlueMatt commented Sep 21, 2017

jl2012 commented Jul 30, 2017 •

edited