New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jan 20, 2016

Conversation

Projects
None yet
9 participants
@sdaftuar
Member

sdaftuar commented Dec 16, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Dec 16, 2015

Member

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.

Member

jonasschnelli commented Dec 16, 2015

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.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Dec 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.

Member

luke-jr commented Dec 17, 2015

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

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Dec 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.

Contributor

petertodd commented Dec 17, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Dec 17, 2015

Member

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

Member

jonasschnelli commented Dec 17, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Dec 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)

Contributor

petertodd commented Dec 17, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Dec 17, 2015

Member

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.

Member

jonasschnelli commented Dec 17, 2015

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

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Dec 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.

Contributor

petertodd commented Dec 17, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Dec 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.

Member

sdaftuar commented Dec 17, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

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

Member

sdaftuar commented Dec 17, 2015

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

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Dec 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.

Contributor

petertodd commented Dec 17, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Dec 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?

Member

sdaftuar commented Dec 18, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Dec 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.

Contributor

dgenr8 commented Dec 20, 2015

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

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Jan 3, 2016

Member

needs rebase.

Member

btcdrak commented Jan 3, 2016

needs rebase.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 4, 2016

Member

Closing in favor of #7292.

Member

sdaftuar commented Jan 4, 2016

Closing in favor of #7292.

@sdaftuar sdaftuar closed this Jan 4, 2016

@sdaftuar sdaftuar reopened this Jan 18, 2016

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jan 18, 2016

Member

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.

Member

gmaxwell commented Jan 18, 2016

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 18, 2016

Member

Needs rebase

Member

laanwj commented Jan 18, 2016

Needs rebase

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

@sdaftuar sdaftuar changed the title from [WIP] RPC: Indicate which transactions are signaling opt-in RBF to RPC: Indicate which transactions are signaling opt-in RBF Jan 18, 2016

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 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.

Member

sdaftuar commented Jan 18, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 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.

Member

sdaftuar commented Jan 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 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.

Member

sdaftuar commented Jan 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 19, 2016

Member

Needs rebase

Member

MarcoFalke commented Jan 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 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.

Member

sdaftuar commented Jan 19, 2016

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 20, 2016

Member

utACK eaa8d27

Member

laanwj commented Jan 20, 2016

utACK eaa8d27

@laanwj laanwj merged commit eaa8d27 into bitcoin:master Jan 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 20, 2016

Merge #7222: RPC: Indicate which transactions are signaling opt-in RBF
eaa8d27 RPC: indicate which transactions are replaceable (Suhas Daftuar)

laanwj added a commit that referenced this pull request Jan 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment