rpc: [mempool] Remove erroneous Univalue integral casts #34112
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34112. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsNo conflicts as of last run. |
|
(This bugfix does not need backport) |
|
Force pushed more tests, to catch more issues: Also, remove all casts in the file completely. |
faf749f to
fa3f24a
Compare
rkrux
left a comment
There was a problem hiding this comment.
tACK fa3f24a
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.
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
index d4bc0a29db..bee16b7587 100644
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -315,7 +315,7 @@ static std::vector<RPCResult> MempoolEntryDescription()
void AppendChunkInfo(UniValue& all_chunks, FeePerWeight chunk_feerate, std::vector<const CTxMemPoolEntry *> chunk_txs)
{
UniValue chunk(UniValue::VOBJ);
- chunk.pushKV("chunkfee", ValueFromAmount(chunk_feerate.fee));
+ chunk.pushKV("chunkfee", ValueFromAmount((int)chunk_feerate.fee));
chunk.pushKV("chunkweight", chunk_feerate.size);
UniValue chunk_txids(UniValue::VARR);
for (const auto& chunk_tx : chunk_txs) {raise AssertionError("not(%s == %s)\n in particular not(%s == %s)" % (thing1, thing2, d1, d2))
AssertionError: 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']}]})
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.
| self.log.info("Test that a large fee delta is honoured") | ||
| tx = self.wallet.create_self_transfer() | ||
| txid = tx["txid"] | ||
| fee_delta = int(41 * COIN) |
There was a problem hiding this comment.
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:
- fee_delta = int(41 * COIN)
+ fee_delta = int(22 * COIN)Passes:
- fee_delta = int(41 * COIN)
+ fee_delta = int(21 * COIN)There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, the comment helps.
I suppose 43 * COIN, which is greater than max(uint32), would have sufficed as well - to make it closer to the limit unless I missed something, but 86 * COIN is fine too.
fa3f24a to
fab1f4b
Compare
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. |
…asts fa66e2d refactor: [rpc] Remove confusing and brittle integral casts (MarcoFalke) Pull request description: When constructing an UniValue from integral values, historically (long ago), in some cases casts where needed. With the current UniValue constructor, only very few are actually needed. Keeping the unused casts around is: * confusing, because code readers do not understand why they are needed * brittle, because some may copy them into new places, where they will lead to hard-to-find logic bugs, such as the ones fixed in pull #34112 So fix all issues by removing them, except for a few cases, where casting was required: * `ret.pushKV("coinbase", static_cast<bool>(coin->fCoinBase));`, or * `static_cast<std::underlying_type_t<decltype(info.nServices)>>(info.nServices)`. This hardening refactor does not fix any bugs and does not change any behavior. ACKs for top commit: sedited: ACK fa66e2d rkrux: ACK fa66e2d Tree-SHA512: 13c9c59ad021ea03cdabe10d58850cef96d792634c499e62227cc2e7e5cace066ebd9a8ef3f979eaba98cadf8a525c6e6df909a07115559c0450bd9fc3a9763e
pablomartin4btc
left a comment
There was a problem hiding this comment.
ACK fab1f4b
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).
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:
Fix all issues by removing the erroneous casts, and by adding a test to check against regressions.