Rename and comment priority calculation in TxMemPoolEntry

[morcos]

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.

[laanwj]

Thanks, better documentation is always welcome.
utACK.

[jonasschnelli]

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.

[luke-jr]

I didn't understand these comments... until I read the code. Not sure how to make it clearer, though.

[penguin1333]

@luke-jr Same story here.

[ashleyholman]

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.

[morcos]

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.

[ashleyholman]

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.

[morcos]

@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...

[morcos]

@ashleyholman See #6357 for what I had in mind....

[morcos]

Closing this in favor of at least #7008