Replace struct update_fee_delta with lambda #24625

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2203-refactor-fee-delta-🔶 changing 3 files +8 −18

MarcoFalke commented at 10:41 AM on March 21, 2022: member

The same was done for another struct in e177fcab3831b6d259da5164cabedcc9e78f6957.

Also, change type of feeDelta from int64_t to CAmount.
Replace struct update_fee_delta with lambda fa8857c3f7
MarcoFalke added the label Refactoring on Mar 21, 2022
MarcoFalke added the label Mempool on Mar 21, 2022
hebasto commented at 11:51 AM on March 21, 2022: member

Concept ACK.
DrahtBot commented at 12:03 PM on March 21, 2022: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #23418 (Fix signed integer overflow in prioritisetransaction RPC by MarcoFalke)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/txmempool.h:134 in fa131d3630 outdated

 130 | @@ -131,7 +131,7 @@ class CTxMemPoolEntry
 131 |      std::chrono::seconds GetTime() const { return std::chrono::seconds{nTime}; }
 132 |      unsigned int GetHeight() const { return entryHeight; }
 133 |      int64_t GetSigOpCost() const { return sigOpCost; }
 134 | -    int64_t GetModifiedFee() const { return nFee + feeDelta; }
 135 | +    CAmount GetModifiedFee() const { return nFee + feeDelta; }

hebasto commented at 12:30 PM on March 21, 2022:

Also suggesting to add:

--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -45,7 +45,7 @@ struct CTxMemPoolModifiedEntry {
         nSigOpCostWithAncestors = entry->GetSigOpCostWithAncestors();
     }
 
-    int64_t GetModifiedFee() const { return iter->GetModifiedFee(); }
+    CAmount GetModifiedFee() const { return iter->GetModifiedFee(); }
     uint64_t GetSizeWithAncestors() const { return nSizeWithAncestors; }
     CAmount GetModFeesWithAncestors() const { return nModFeesWithAncestors; }
     size_t GetTxSize() const { return iter->GetTxSize(); }

MarcoFalke commented at 12:38 PM on March 21, 2022:

Thx, done

hebasto approved
hebasto commented at 12:30 PM on March 21, 2022: member

ACK fa131d3630bb8da6816e50a9a37c0ed4ad4ee33c, I have reviewed the code.
Use CAmount for fee delta and modified fee fa84a49526
MarcoFalke force-pushed on Mar 21, 2022
hebasto approved
hebasto commented at 12:39 PM on March 21, 2022: member

re-ACK fa84a49526fcf76e98b0b2f4d4b00b34a8dddf46
promag commented at 9:49 PM on March 21, 2022: member

Code review ACK fa84a49526fcf76e98b0b2f4d4b00b34a8dddf46.
promag commented at 10:28 PM on March 21, 2022: member

Feel free to pull https://github.com/MarcoFalke/bitcoin-core/compare/2203-refactor-fee-delta-%F0%9F%94%B6...promag:2203-refactor-fee-delta-%F0%9F%94%B6

https://github.com/promag/bitcoin/commit/b440b03845410c2892f73497f774d29e897caed7 https://github.com/promag/bitcoin/commit/c5cdfe56de13fefb56f90d1af2f7912b0c19f1a6
MarcoFalke commented at 6:39 AM on March 22, 2022: member

I didn't touch the other structs, as they take multiple arguments. I think before touching them, we should ideally decide whether to use designated initializers or clang-tidy named arguments.
fanquake commented at 12:48 PM on March 24, 2022: member

@glozow want to review?
glozow commented at 3:23 PM on March 24, 2022: member

Concept ACK, haven't reviewed code
fanquake merged this on Mar 24, 2022
fanquake closed this on Mar 24, 2022
MarcoFalke deleted the branch on Mar 24, 2022
sidhujag referenced this in commit c93c70ac8d on Apr 2, 2022
Fabcien referenced this in commit a71672691a on Nov 23, 2022
DrahtBot locked this on Mar 24, 2023

Contributors

Labels

Replace struct update_fee_delta with lambda #24625

Conflicts