Casting without reason can only be confusing (because it is not needed), or wrong (because it does the wrong thing).
For example, the added test that adds a positive chunk prioritization will fail:
0AssertionError: not(-1.94936096 == 41.000312)
Fix all issues by removing the erroneous casts, and by adding a test to check against regressions.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34112.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | rkrux, pablomartin4btc, glozow |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
No conflicts as of last run.
Force pushed more tests, to catch more issues:
0 in particular not({'chunks': [{'chunkfee': Decimal('-1.94936096'), 'chunkweight': 415, 'txs': ['2db3a22f9370eb32f110597882acebef1c76cf319837bac69b829917730d2349']}]} == {'chunks': [{'chunkfee': Decimal('41.000312'), 'chunkweight': 415, 'txs': ['2db3a22f9370eb32f110597882acebef1c76cf319837bac69b829917730d2349']}]})
Also, remove all casts in the file completely.
35@@ -36,6 +36,57 @@ def clear_prioritisation(self, node):
36 node.prioritisetransaction(txid, 0, -delta)
37 assert_equal(node.getprioritisedtransactions(), {})
38
39+ def test_large_fee_bump(self):
40+ self.log.info("Test that a large fee delta is honoured")
41+ tx = self.wallet.create_self_transfer()
42+ txid = tx["txid"]
43+ fee_delta = int(41 * COIN)
Curious why 41 was chosen? I also tested with values much closer to the typical max value of a signed int in cpp.
With the above patch that I mentioned in the overall review comment, the test behaviour is like the following -
Fails:
0- fee_delta = int(41 * COIN)
1+ fee_delta = int(22 * COIN)
Passes:
0- fee_delta = int(41 * COIN)
1+ fee_delta = int(21 * COIN)
Good question. I guess we want to have something that does not fit into both i32 and u32, so may as well pick 86*COIN?
I’ve also added a comment. Let me know if this makes it clearer.
tACK fa3f24a407049a0a15edf288a94749edbb3439f2
In strong agreement with removing unnecessary casts from a code readability POV as well.
I applied the following patch to see if the test fails as mentioned and got the below error.
0diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
1index d4bc0a29db..bee16b7587 100644
2--- a/src/rpc/mempool.cpp
3+++ b/src/rpc/mempool.cpp
4@@ -315,7 +315,7 @@ static std::vector<RPCResult> MempoolEntryDescription()
5 void AppendChunkInfo(UniValue& all_chunks, FeePerWeight chunk_feerate, std::vector<const CTxMemPoolEntry *> chunk_txs)
6 {
7 UniValue chunk(UniValue::VOBJ);
8- chunk.pushKV("chunkfee", ValueFromAmount(chunk_feerate.fee));
9+ chunk.pushKV("chunkfee", ValueFromAmount((int)chunk_feerate.fee));
10 chunk.pushKV("chunkweight", chunk_feerate.size);
11 UniValue chunk_txids(UniValue::VARR);
12 for (const auto& chunk_tx : chunk_txs) {
0raise AssertionError("not(%s == %s)\n in particular not(%s == %s)" % (thing1, thing2, d1, d2))
1AssertionError: not({'clusterweight': 415, 'txcount': 1, 'chunks': [{'chunkfee': Decimal('-1.94936096'), 'chunkweight': 415, 'txs': ['2db3a22f9370eb32f110597882acebef1c76cf319837bac69b829917730d2349']}]} == {'clusterweight': 415, 'txcount': 1, 'chunks': [{'chunkfee': Decimal('41.000312'), 'chunkweight': 415, 'txs': ['2db3a22f9370eb32f110597882acebef1c76cf319837bac69b829917730d2349']}]})
2 in particular not({'chunks': [{'chunkfee': Decimal('-1.94936096'), 'chunkweight': 415, 'txs': ['2db3a22f9370eb32f110597882acebef1c76cf319837bac69b829917730d2349']}]} == {'chunks': [{'chunkfee': Decimal('41.000312'), 'chunkweight': 415, 'txs': ['2db3a22f9370eb32f110597882acebef1c76cf319837bac69b829917730d2349']}]})
Also, went through the changes where the cast is removed and matched with the return type of the functions whose value was cast - made sense to me in all the cases to remove.
I applied the following patch to see if the test fails as mentioned and got the below error. […] Also, went through the changes where the cast is removed and matched with the return type of the functions whose value was cast - made sense to me in all the cases to remove.
Sorry, it was a bit confusing on my part to mix random refactors with bugfixes. However, there are two bugfixes. I’ve moved the refactors to #34113, so that this only contains the two bugfixes.
tACK fab1f4b800d007bd4756b2519c64e1506ffe0d6c
Thanks for moving the refactors in a separate pull.
Edit: I just realised that how rare would it be to face this error in mainnet, but better to have bugs not lying around in the codebase.
ACK fab1f4b800d007bd4756b2519c64e1506ffe0d6c
ValueFromAmount expects a full-range CAmount (semantically and type-wise chunk_feerate.fee is); casting to int narrows the value to 32 bits, risking silent truncation and incorrect RPC output, so the cast is both unnecessary and unsafe.
The 2 casts were added and merged recently (#33591 & #33629 - one line each).
