Rename and comment priority calculation in TxMemPoolEntry #6292

1. 
   morcos commented at 6:41 pm on June 16, 2015: member

   The GetPriority calculation in CTxMemPoolEntry is incorrect. Instead of a more complicated and extensive fix which may be unnecessary, just rename the function and document its current behavior.
2. Rename and comment priority calculation in TxMemPoolEntry to document its limitations. 1386ff5df2
3. 
   laanwj commented at 6:36 am on June 17, 2015: member

   Thanks, better documentation is always welcome. utACK.
4. 
   jonasschnelli commented at 6:57 am on June 17, 2015: contributor

   utACK.

   Short, unclear variable- and function names and missing comments are probably one of the main obstacle for new contributors to better understand the code.

5. 
   luke-jr commented at 6:32 am on June 23, 2015: member

   I didn’t understand these comments… until I read the code. Not sure how to make it clearer, though.
6. 
   penguin1333 commented at 5:49 pm on June 23, 2015: none

   @luke-jr Same story here.
7. 
   ashleyholman commented at 6:38 am on June 25, 2015: contributor

   I just realised this bug effects my pull request #6331 because I am relying on GetPriority() to give correct results.

   How about fixing the bug? I don’t think it needs a complicated fix. Just change GetPriority to calculate the priority of each of the inputs. For unconfirmed inputs, they count as 0. It’s a more expensive calculation than the current method, but with my PR the calculation call will only need to be done once each time the active chain changes, and then the result is cached in CTxMemPoolEntry.

   I’m happy to include a fix in my PR if people agree.

8. 
   morcos commented at 3:29 pm on June 25, 2015: member

   Hmm. I actually think that would be too expensive to do it directly. I had an idea for how to keep an on-the-fly calculation that would be more efficient. I wouldn’t mind implementing it or suggesting it for you to implement, but the big IF here is whether all this complication is necessary. Even if this priority calculation were cheap, is looping the entire mempool on every block to update priorities something we actually want to do?

   I’d be more in favor of ditching the priority sorting and free transaction inclusion policy altogether. I’m not sure what purpose it serves especially as blocks are closer to getting full.

9. 
   ashleyholman commented at 1:43 am on June 27, 2015: contributor

   edit: I get your point now.. dropping the priority calculation altogether would definitely simplify things.

   Aside from that.. what was your idea for more efficient on-the-fly priority calculation? I had thought about using the current ‘delta’ technique but making it correct by tracking the confirmation state of the inputs, but that sounds quite complex to implement.

   Intuitively I thought that a recalcing the priorities of all mempool inputs would be fairly fast, but I’ll measure it on my crappy vm-inside-old-macbook and see how long it takes.

10. 
    morcos commented at 3:10 pm on June 30, 2015: member

    @ashleyholman. yes that was my idea. when a block comes in, you know which transactions in the mempool depend on each of the newly confirmed transactions, so you can just go update the current priority of those transactions and give them a new calculated priority, the height it was calculated at, and new delta for future priorities. Maybe I’ll take a pass at it…
11. 
    morcos commented at 8:27 pm on June 30, 2015: member

    @ashleyholman See #6357 for what I had in mind….
12. laanwj added the label Mempool on Jul 2, 2015
13. 
    morcos commented at 7:03 pm on November 14, 2015: member

    Closing this in favor of at least #7008
14. morcos closed this on Nov 14, 2015

    ---

15. DrahtBot locked this on Sep 8, 2021

Contributors
morcos laanwj jonasschnelli luke-jr penguin1333 ashleyholman

Labels
Mempool