RPC: Indicate which transactions are signaling opt-in RBF

sdaftuar commented at 8:06 pm on December 16, 2015: member

I’m not sure how users of bitcoind are expected to figure out which transactions could be replaced via opt-in RBF, so I took a first stab at exposing that via RPC. I’ve started by adding it to getrawmempool when called with the verbose=true argument. I’ve also exercised that code with some small modifications to the replace-by-fee.py rpc test.

I’d appreciate some specific feedback on the following:

Which RPC calls should return this information?
Would it be helpful to distinguish between a transaction signaling RBF itself (with nSequence < 0xfffffffe on one of its inputs), versus inheriting the signal from some unconfirmed parent?
The answer can only be definitive for transactions that are in the mempool; transactions that are not in the mempool might have unknown parents which could signal RBF and we wouldn’t know it. Is it worth trying to answer this question for transactions not in the mempool, and if so, what information should be returned?

jonasschnelli commented at 8:49 pm on December 16, 2015: contributor

Concept ACK

Which RPC calls should return this information?

I think at least listtransaction, gettransaction (wallet functions) should also support this.

Would it be helpful to distinguish between a transaction signaling RBF itself […], versus inheriting the signal from some unconfirmed parent?

Yes. This would be useful IMO.

The answer can only be definitive for transactions that are in the mempool; transactions that are not in the mempool might have unknown parents which could signal RBF and we wouldn’t know it. Is it worth trying to answer this question for transactions not in the mempool, and if so, what information should be returned?

I don’t see a use-case for getting the RBF signal for confirmed transactions (maybe for potential reorgs risk calculations?). But I agree, without -txindex, parents RBF signal is probably unknown.

jonasschnelli added the label RPC on Dec 16, 2015

luke-jr commented at 1:38 am on December 17, 2015: member

I’m not sure how users of bitcoind are expected to figure out which transactions could be replaced via opt-in RBF,

I don’t see any use case for exposing this information.

petertodd commented at 7:39 am on December 17, 2015: contributor

NACK

Very high risk of naive users attempting to use this to do risk mitigation and accepting zeroconf txs in cases where they’re risking losses; if you care about whether a tx can be replaced you’re in a position where you should be using specialist services.

Note how easy it is to send replacable in practice txs by just having an ancestor tx have a low fee.

jonasschnelli commented at 8:43 am on December 17, 2015: contributor

@petertodd: for wallet users (listtransaction, gettransaction): I think there is a use case for self created wtxs where a user likes to know whether he did enable RBF or not.

petertodd commented at 9:13 am on December 17, 2015: contributor

True, but for those users knowing if a tx is RBF is already trivial - just check nSequence. Equally, we haven’t even merged my obvious set RBF flag option, so what’s the usecase?

On 17 December 2015 00:44:28 GMT-08:00, Jonas Schnelli notifications@github.com wrote:

@petertodd: for wallet users (listtransaction, gettransaction): I think there is a use case for self created wtxs where a user likes to know whether he did enable RBF or not.

Reply to this email directly or view it on GitHub: #7222 (comment)

jonasschnelli commented at 9:22 am on December 17, 2015: contributor

Equally, we haven’t even merged my obvious set RBF flag option, so what’s the use case?

Right. This (an option to set the RBF flag when creating a wtx) would be required first. Either your overall-setting PR (#7132) or the one that supports rawtx (#7159). GUI option might follow soon.

[…] just check nSequence

IMO this is basically what this PR does.

petertodd commented at 12:21 pm on December 17, 2015: contributor

No, this pull-req checks ancestors too, a critical difference thats applicable to “risk-scoring” BS but has little applicibility to your own transactions.

sdaftuar commented at 2:45 pm on December 17, 2015: member

@petertodd I don’t agree, but I can understand your concern about offering a feature that could mislead users. So rather than change existing RPC calls as I initially proposed, what about just adding two new RPC calls, one that would return the txid’s of unconfirmed parents of a transaction and one that would return txid’s of in-mempool descendants? That seems to me to be more broadly useful and basically addresses my underlying concern, that users who want to do calculations relating to whether a transaction might be replaced would have to reinvent the wheel to efficiently trace mempool chains.

sdaftuar commented at 2:46 pm on December 17, 2015: member

I should add – I think exposing mempool ancestors/descendants is also useful for code that would seek to use RBF efficiently (I’m not sure how else you’d figure out what fee to use, or whether you’ll get rejected because you’re replacing too many transactions, etc).

petertodd commented at 11:55 pm on December 17, 2015: contributor

Is efficiency in that case actually a concern? I’d rather see that version proposed in conjunction with code that actually needs it, and benchmarks showing how badly it’s needed.

sdaftuar commented at 0:44 am on December 18, 2015: member

How would an application even calculate what fee to use to rbf two transactions down to one? The only thing I can think of would be to call getrawmempool and recursively search for transactions which depend on the given transactions. Dumping a 300mb mempool over rpc sounds like a losing proposition to begin with, never mind reimplementing the logic bitcoind already has for iterating.

Is there a simpler approach I’m missing?

dgenr8 commented at 9:45 pm on December 20, 2015: contributor

Concept ACK. The arguments against making this (or any basic tx attribute information) available to a user who is explicitly looking for it are beyond unconvincing. As users, we want to see what we just did. And we want to see details about the tx that just arrived that pays us. RPC’s to return ancestors and descendants would be extremely useful too.

Re: throwing a runtime error in IsRBFOptIn(...), maybe make it three-valued (yes, no, unknown)? Caller will probably end up doing that anyway.

btcdrak commented at 8:10 pm on January 3, 2016: contributor

needs rebase.

sdaftuar commented at 9:12 pm on January 4, 2016: member

Closing in favor of #7292.

sdaftuar closed this on Jan 4, 2016

sdaftuar reopened this on Jan 18, 2016

gmaxwell commented at 3:59 pm on January 18, 2016: contributor

Pardon my tardy response. With due respect to the prior arguments, they’re more strongly an argument against showing unconfirmed transactions at all by default. Not against this.

An indicator for BIP125 (in particular, in listtransactions) would be both labor saving for users, and also make it more likely that parties looking at it get it right (and, for example, don’t consider confirmed transactions to be BIP125 mempool replaceable).

Moreover, it was always my expectation that it would work this way– and this small affordance is an aspect of how I explained BIP125 to others without getting any protest so I expect many also expected this. Importantly, while many people do objectively confused things with unconfirmed transactions, the elegance of BIP125 was that it avoided the difficult task of educating people which so far many have failed with; by providing something that allowed their activities to continue without conflict. But it doesn’t do so if getting access to the information is too burdensome. Because of this I consider this a release blocker.

I was concerned that the patch for this would be complex, but it turns out to be quite clean… which I think makes this a no-brainer.

laanwj commented at 4:05 pm on January 18, 2016: member

Needs rebase

laanwj added this to the milestone 0.12.0 on Jan 18, 2016

sdaftuar force-pushed on Jan 18, 2016

sdaftuar renamed this:
~~[WIP] RPC: Indicate which transactions are signaling opt-in RBF~~
RPC: Indicate which transactions are signaling opt-in RBF
on Jan 18, 2016

sdaftuar commented at 8:21 pm on January 18, 2016: member

Rebased and updated to extend this information to listtransactions and gettransaction. I also updated the listtransactions.py rpc test to exercise this code.

This appears to merge cleanly into 0.12 as well.

sdaftuar force-pushed on Jan 19, 2016

sdaftuar commented at 3:17 am on January 19, 2016: member

On further thought I’ve removed the opt-in RBF signaling information in getrawmempool. I timed that RPC call – without this change – on a node with a large mempool and it took over 6seconds. Potentially multiplying that by the average chain length (limit of 25 by default) seems like a bad performance hit to suddenly introduce.

I had an issue with the build not working after pulling the code out of rpcblockchain.cpp; the second commit which moves policy/rbf.cpp to libbitcoin_wallet seems to be a workaround. I imagine there may be a better way of fixing the build, but leaving it here for now to make travis happy and so the rest of this pull can be reviewed. We can replace or squash the second commit as appropriate… @cfields Thanks for looking at this; let me know if you figure out a better way to make everything work.

sdaftuar commented at 11:39 am on January 19, 2016: member

Sorry for the churn – changed the name of the field in the RPC output to be “bip125-replaceable”, and fixed the handling of confirmed transactions.

MarcoFalke commented at 1:15 pm on January 19, 2016: member

Needs rebase

RPC: indicate which transactions are replaceable

Add "bip125-replaceable" output field to listtransactions and gettransaction
which indicates if an unconfirmed transaction, or any unconfirmed parent, is
signaling opt-in RBF according to BIP 125.

eaa8d2754b

sdaftuar force-pushed on Jan 19, 2016

sdaftuar commented at 1:39 pm on January 19, 2016: member

Rebased (trivial conflict with #7164 in listtransactions.py). Also squashed down to one commit, and verified this still merges cleanly into 0.12.

laanwj commented at 12:40 pm on January 20, 2016: member

utACK eaa8d27

laanwj merged this on Jan 20, 2016

laanwj closed this on Jan 20, 2016

laanwj referenced this in commit 82429d0861 on Jan 20, 2016

laanwj referenced this in commit 7c5e90e8dd on Jan 22, 2016

DrahtBot locked this on Sep 8, 2021

RPC: Indicate which transactions are signaling opt-in RBF #7222