Track modified size in TxMemPoolEntry to correctly compute priority #4817

pull morcos wants to merge 1 commits into bitcoin:master from morcos:fix-GetPriority changing 4 files +17 −3

morcos commented at 2:36 PM on September 2, 2014: member

CTxMemPoolEntry::GetPriority was incorrectly using the full size of a transaction to caclulate a priority delta to figure out current priority as opposed to priority when the transaction entered the mempool. I added nModSize to CTxMemPoolEntry and fixed GetPriority.

in src/txmempool.cpp:None in 9bc8c20bfe outdated

  22 | @@ -23,6 +23,14 @@ CTxMemPoolEntry::CTxMemPoolEntry(const CTransaction& _tx, int64_t _nFee,
  23 |      tx(_tx), nFee(_nFee), nTime(_nTime), dPriority(_dPriority), nHeight(_nHeight)
  24 |  {
  25 |      nTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION);
  26 | +
  27 | +    nModSize = nTxSize;
  28 | +    BOOST_FOREACH(const CTxIn& txin, tx.vin)
  29 | +    {
  30 | +        unsigned int offset = 41U + std::min(110U, (unsigned int)txin.scriptSig.size());

gavinandresen commented at 3:03 PM on September 2, 2014:

Can you refactor so this isn't repeated in txmempool.cpp and core.cpp? If not, then a comment in both places is in order, so if we decide it should be 42U + ... we update in both places.

morcos commented at 7:06 PM on September 2, 2014: member

OK I added a function CalculateModifiedSize to CTransaction and now that code is only called once. I find the plethora of priority calculating functions (CTransaction::ComputePriority, CConsViewCache::GetPriority, CTxMemPoolEntry::GetPriority) a bit hard to follow on first look, but I couldn't see any reasonable way to clean them up. Should I get rid of the GetModSize() accessor since it's not used?
gavinandresen commented at 7:10 PM on September 2, 2014: contributor

If something isn't used, yes, get rid of it!
TheBlueMatt commented at 6:46 AM on September 8, 2014: member

Maybe you can squash these commits? Still, ut ACK.
sipa commented at 1:18 PM on September 8, 2014: member

untested ACK
Track modified size in TxMemPoolEntry so that we can correctly compute priority. c26649f9ed
morcos force-pushed on Sep 8, 2014
morcos commented at 3:32 PM on September 8, 2014: member

Ok squashed
BitcoinPullTester commented at 3:50 PM on September 8, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4817_c26649f9ed03fa9505e44aaf7f8cfdaa81f734cc/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
jgarzik commented at 1:59 AM on September 15, 2014: contributor

ut ACK
sipa merged this on Sep 15, 2014
sipa closed this on Sep 15, 2014
sipa referenced this in commit 2ec82e94e6 on Sep 15, 2014
morcos deleted the branch on Nov 12, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors