rpc: Report reason for replaceable txpool transactions #16490
Conversation
maflcko
force-pushed
the
1907-rpcMempoolWhyReplacable
branch
from
fa816db to
fa32d57
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
maflcko
force-pushed
the
1907-rpcMempoolWhyReplacable
branch
from
fa32d57 to
fad333b
Compare
|
Concept ACK |
maflcko
force-pushed
the
1907-rpcMempoolWhyReplacable
branch
from
fad333b to
fa12ae2
Compare
maflcko
force-pushed
the
1907-rpcMempoolWhyReplacable
branch
3 times, most recently
from
faa21f2 to
fa728ba
Compare
|
Added test |
|
BIP 125 is pretty straightforward... Any other policy isn't BIP 125. It would be incorrect for If we want to be clearer with regard to different policies, we need a new boolean too. Or just a string/null field. A string that doesn't answer the main "is this replacable?" question isn't very helpful. |
Also, avoid direct access to mapTx
maflcko
force-pushed
the
1907-rpcMempoolWhyReplacable
branch
3 times, most recently
from
fa0104d to
fa0bead
Compare
|
Ok, made it a new optional string, without modifying existing return values |
maflcko
changed the title
There was a problem hiding this comment.
tested and code review ACK
Conceptually, I think I would have liked it a little more if replaceable was a boolean and then there was another string field replaceable-reason, similar to reject-reason in the testmempoolaccept RPC. Is it realistic that we could remove bip125-replaceable then in the future? If that is an option I would prefer it but I don't know how many users might rely on bip125-replaceable.
maflcko
force-pushed
the
1907-rpcMempoolWhyReplacable
branch
from
fa0bead to
fa35085
Compare
maflcko
force-pushed
the
1907-rpcMempoolWhyReplacable
branch
from
fa35085 to
fa5d55d
Compare
|
Ok, changed to what @fjahr suggested |
|
tested ACK fa5d55d |
|
This has three ACKs, so I will probably merge it in the next few days unless there are objections. |
|
This change seems premature to me -- it only seems to make sense in a world where we've updated our actual policy rules to include other-than-bip-125 replacement, which we've not done (yet). In the meantime, having an extra "replaceable" field on top of "bip-125-replaceable" seems like it will confuse our users. I don't think we should make a change like this in a release where bip 125 replacement is the only thing we support. |
Might be less confusing to mark the old field as deprecated, and maybe start the process of removing it with deprecation flags. If policy rules are likely to be updated in the future, updating the RPC interface sooner that could allow writing more robust RPC client code that won't have to change later. If I were writing RPC client code and knew there were going to be changes to the interface, I'd want more time to develop and test against those changes rather than less. |
|
@ryanofsky If we had consensus to change our policy rules, then I'd agree that marking the old field as deprecated would make sense to allow people to prepare. But I think the issue has been that we haven't reached that consensus yet (I believe several PRs in the last year or two that proposed new replacement policies have stalled out or been closed). |
|
Achieving consensus about changing policy rules and achieving consensus about future-proofing an RPC seem like things that could be done separately. That'd probably be my inclination, so I'd keep my previous ACK here. But I don't have a very strong opinion, and really just wanted to add the practical suggestion to document the less general field as deprecated to avoid confusion, or at least mention the difference between the two fields in the documentation. |
| } | ||
|
|
||
| boost::optional<CTxMemPool::txiter> CTxMemPool::GetIter(const uint256& txid) const | ||
| Optional<CTxMemPool::txiter> CTxMemPool::GetIter(const uint256& txid) const |
There was a problem hiding this comment.
Please rebase as this change is already in master.
| Needs rebase |
| // If this transaction is not in our mempool, then we can't be sure | ||
| // we will know about all its inputs. | ||
| if (!pool.exists(tx.GetHash())) { | ||
| const Optional<CTxMemPool::txiter> it = pool.GetIter(tx.GetHash()); |
11 participants
Currently, the reason for the boolean
bip125-replaceableis not given. However, future versions of Bitcoin Core (or software forks of Bitcoin Core) might offer different mempool (replacement) policies to their users. So instead of a flat boolean, it would be nice to return the reason whybip125-replaceableis true or false. This pull does that among a refactoring commit.