fix(rpc): Avoid selecting duplicate transactions in block templates

[arya2]

Motivation

The block template should not contain duplicate transactions.

Closes #5982.

Solution

• Logs hashes of mempool transactions returned by FullTransactions request.
• Logs selected mempool transaction hashes and unpaid actions.
• Removes candidate transactions from candidate transaction collections during selection.

Review

Part of regular scheduled work.

Reviewer Checklist

• Will the PR name make sense to users?
  • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
• Are the PR labels correct?
• Does the code do what the ticket and PR says?
  • Does it change concurrent code, unsafe code, or consensus rules?
• How do you know it works? Does it have tests?

[codecov]

Codecov Report

Merging #6006 (b1cec4f) into main (67194c4) will decrease coverage by 0.07%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6006      +/-   ##
==========================================
- Coverage   78.01%   77.94%   -0.07%     
==========================================
  Files         312      312              
  Lines       38997    38997              
==========================================
- Hits        30425    30398      -27     
- Misses       8572     8599      +27     

[teor2345]

Looks good, just a few tweaks for usability and maintenance.

How do you know this fix works?
Can you find a second reviewer for this PR?

[arya2]

Looks good, just a few tweaks for usability and maintenance.

How do you know this fix works?

It failed about 10 in ~30 tests before the change and 0 in ~20 tests after the change.

I checked the logic for the mempool and it shouldn't be returning any duplicate transactions, I also checked the hashes returned by the FullTransactions request several times to be sure that the duplicates were from the selection logic.

The new selection logic doesn't clone candidate transactions anymore so I think that guarantees against duplication there.

I tried to be thorough in confirming that the new logic works, I think update_weights wasn't behaving as expected due to subtracting floating point values from the total weight.

Can you find a second reviewer for this PR?

Good idea, I added @oxarbitrage as a reviewer.

[oxarbitrage]

I think it looks good, i made a few manual calls and got no duplicates.

I was thinking on making a script that just runs getblocktemplate and check there are no duplicate transactions however i am not really sure if it worth it.

If we think it does, we can think on the test for it in a separated issue with lower priority.

[teor2345]

Looks good, just a few tweaks for usability and maintenance.
How do you know this fix works?

It failed about 10 in ~30 tests before the change and 0 in ~20 tests after the change.

I checked the logic for the mempool and it shouldn't be returning any duplicate transactions, I also checked the hashes returned by the FullTransactions request several times to be sure that the duplicates were from the selection logic.

The new selection logic doesn't clone candidate transactions anymore so I think that guarantees against duplication there.

I tried to be thorough in confirming that the new logic works, I think update_weights wasn't behaving as expected due to subtracting floating point values from the total weight.

We already have a CI test PR that will catch this bug, so I don't think we need any other tests.