Removed redundant parameter from mempool.PrioritiseTransaction #9801

pull gubatron wants to merge 1 commits into bitcoin:master from gubatron:refactor-mempool-prioritisetx changing 4 files +6 −6

gubatron commented at 5:31 PM on February 19, 2017: contributor

The string parameter is used only for logging.

The only place where the string form of the hash isn't recalculated is at src/rpc/mining.cpp where it's obtained from the arguments, however, in this case the uint256 hash is then parsed from the given string.

In every other use of the function, the string representation is always calculated from the uint256 hash object (either with ToString() or with GetHash()), so no real benefit on having this redundant parameter.

This also makes the API more consistent with other methods that only receive the uint256 hash parameter.
jonasschnelli approved
jonasschnelli commented at 7:27 PM on February 19, 2017: contributor

Makes sense. Thanks. utACK 8e88a445dc9de71657282a461cb6b679c9079ebd
jonasschnelli added the label Mempool on Feb 19, 2017
jonasschnelli added the label Refactoring on Feb 19, 2017
paveljanik commented at 8:20 PM on February 19, 2017: contributor

ACK https://github.com/bitcoin/bitcoin/commit/8e88a445dc9de71657282a461cb6b679c9079ebd
dcousens approved
MarcoFalke commented at 2:16 AM on February 20, 2017: member

utACK 8e88a44

in src/txmempool.cpp:None in 8e88a445dc outdated

 919 | @@ -920,7 +920,7 @@ CTxMemPool::ReadFeeEstimates(CAutoFile& filein)
 920 |      return true;
 921 |  }
 922 |  
 923 | -void CTxMemPool::PrioritiseTransaction(const uint256 hash, const std::string strHash, double dPriorityDelta, const CAmount& nFeeDelta)
 924 | +void CTxMemPool::PrioritiseTransaction(const uint256 hash, double dPriorityDelta, const CAmount& nFeeDelta)

sdaftuar commented at 8:17 PM on February 21, 2017:

Perhaps while you're at it you could make this const uint256& hash?

gubatron commented at 10:23 PM on February 21, 2017:

done deal, thanks.

(squashed and rebased)

Removed redundant parameter from mempool.PrioritiseTransaction

(Also made the `const uint256 hash` parameter a `const uint256& hash` as suggested by @sdaftuar)

eaea2bbb41

in src/txmempool.h:None in 95f36bb02f outdated

 554 | @@ -555,7 +555,7 @@ class CTxMemPool
 555 |      bool HasNoInputsOf(const CTransaction& tx) const;
 556 |  
 557 |      /** Affect CreateNewBlock prioritisation of transactions */
 558 | -    void PrioritiseTransaction(const uint256 hash, const std::string strHash, double dPriorityDelta, const CAmount& nFeeDelta);
 559 | +    void PrioritiseTransaction(const uint256& hash, const std::string strHash, double dPriorityDelta, const CAmount& nFeeDelta);

MarcoFalke commented at 11:39 PM on February 21, 2017:

The redundant parameter is not removed, as mentioned in the commit message.

MarcoFalke commented at 11:40 PM on February 21, 2017:

You can run make check locally as a quick sanity check.

gubatron commented at 3:45 AM on February 22, 2017:

yes, something got screwed with the rebase, re-uploaded now. Thanks.

paveljanik commented at 6:55 AM on February 22, 2017: contributor

ACK https://github.com/bitcoin/bitcoin/pull/9801/commits/eaea2bbb411c5d878e991d9082ca037628efce42
MarcoFalke commented at 8:53 AM on February 22, 2017: member

utACK eaea2bb
laanwj merged this on Feb 22, 2017
laanwj closed this on Feb 22, 2017
laanwj referenced this in commit a8c5751038 on Feb 22, 2017
gubatron deleted the branch on Feb 23, 2017
PastaPastaPasta referenced this in commit 3a3745bbe6 on Dec 28, 2018
PastaPastaPasta referenced this in commit 4022c413d3 on Dec 29, 2018
PastaPastaPasta referenced this in commit 4925d9774c on Dec 29, 2018
PastaPastaPasta referenced this in commit 059764a1b3 on Dec 29, 2018
PastaPastaPasta referenced this in commit e94a4c2f78 on Jan 2, 2019
PastaPastaPasta referenced this in commit 56e0a2c02e on Jan 3, 2019
PastaPastaPasta referenced this in commit 5c4ba9f443 on Jan 3, 2019
UdjinM6 referenced this in commit 07dcddb4ca on Jan 7, 2019
DrahtBot locked this on Sep 8, 2021

Contributors

Labels