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.

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.

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.

@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.

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 [email protected] 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)

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.

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

@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.

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).

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.

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?

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.

needs rebase.

Closing in favor of #7292.

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.

Needs rebase

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.

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.

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.

Needs rebase

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

utACK eaa8d27