Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- estimatefee
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 4976 Build 8015: Bitcoin ABC Buildbot (legacy) Build 8014: arc lint + arc unit
Event Timeline
| src/rpc/mining.cpp | ||
|---|---|---|
| 777 | Shouldn't it be > 0, or is it intentional to allow for using the deprecated parameter without returning the help message ? | |
| 783 | More thought: as it is not used anyway, is it really necessary to mark it as deprecated rather than deleting it ? Leaving it would require the help message to be cleaned up later to remove it. | |
| src/wallet/fees.cpp | ||
| 16 | Do you plan to remove nConfirmTarget also (no longer used) ? | |
| src/rpc/mining.cpp | ||
|---|---|---|
| 783 | Originally I was going to clean it up later. But after thinking about your comment above, I think we can just provide a message like: if (request.params.size() == 1) {
// DEPRECATED error message
} | |
| src/qt/sendcoinsdialog.cpp | ||
|---|---|---|
| 790 | It's used in the labelFeeEstimation below. I can still clean this up, but the message will need to change to "next block". | |
| src/wallet/fees.cpp | ||
|---|---|---|
| 16 | I will do this in a separate diff due to the size of that change being larger than this one. | |
| src/rpc/mining.cpp | ||
|---|---|---|
| 783 ↗ | (On Diff #7410) | This is breaking that API with no test and no mention in the release notes and no deprecation cycle. |
| src/rpc/mining.cpp | ||
|---|---|---|
| 788 ↗ | (On Diff #7417) | Unless I'm missing something, you can avoid this by moving blocks of code like this:
|
| test/functional/rpc_estimatefee.py | ||
| 22 ↗ | (On Diff #7417) | Is it safe to compare floats with equality in python ? |
| test/functional/rpc_estimatefee.py | ||
|---|---|---|
| 22 ↗ | (On Diff #7434) | You may have over-engineered it here: from decimal import Decimal
assert_equal(self.nodes[0].estimatefee(), Decimal('0.00001')) |
So it does looks like that you clearly have a diff for the RPC specifically that detaches itself.
In it, you remove some parameter, but you also unmap the parameter name, which is going to break anything using named parameters, even when the deprecation flags are used.
| src/rpc/mining.cpp | ||
|---|---|---|
| 808 ↗ | (On Diff #7443) | It seems like there is some behavior change here. What was this behavior for and why does it need to be removed? |
Fixed named arguments + added a test for it
| src/rpc/mining.cpp | ||
|---|---|---|
| 808 ↗ | (On Diff #7443) | This appears to be a dead codepath since we've done a number of modifications to txmempool::estimateFee(). See the current implementation: CFeeRate CTxMemPool::estimateFee() const {
LOCK(cs);
uint64_t maxMempoolSize =
gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000;
// minerPolicy uses recent blocks to figure out a reasonable fee. This
// may disagree with the rollingMinimumFeerate under certain scenarios
// where the mempool increases rapidly, or blocks are being mined which
// do not contain propagated transactions.
return std::max(GetConfig().GetMinFeePerKB(), GetMinFee(maxMempoolSize));
}Although I suppose it's possible to force this function to return zero with certain arguments, I don't see why a user would do such a thing. As such, it seemed prudent to clean up the implementation. |
This is still doing many things at once. Changing the API of the RPC is more than enough for its own patch for instance.