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] Expose ancestor/descendant information over RPC #7292

Merged
merged 7 commits into from Jun 9, 2016

Conversation

Projects
None yet
9 participants
@sdaftuar
Member

sdaftuar commented Jan 4, 2016

This adds 3 new RPC calls (getancestors getmempoolancestors, getdescendants getmempooldescendants, getmempoolentry) to expose more mempool information over RPC.

Now that we have policy rules that are tied to transaction chains (including limits on the number of in-mempool ancestors and in-mempool descendants, and mempool eviction and RBF policies that depend on fees of descendants), it seems helpful to be able to expose more information about the chains a given tx is part of over RPC.

This is motivated by the discussion in #7222. @petertodd You mentioned wanting to see specific use-cases for this information; I think there are clear improvements to the fee-bumping tools in your replace-by-fee repo that could be made if we had these functions (the first two tools I looked at just now don't seem to consider transaction chains at all in fee calculations?). I'll try to post a link in this PR to specific proposed improvements when I have something to share that will demonstrate this more clearly.

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Jan 5, 2016

Member

Concept ACK

Member

btcdrak commented Jan 5, 2016

Concept ACK

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jan 5, 2016

Member

Concept ACK.
Some concrete use-cases would be nice.
Nice to see the new RPC commands cs_main lock free.

Member

jonasschnelli commented Jan 5, 2016

Concept ACK.
Some concrete use-cases would be nice.
Nice to see the new RPC commands cs_main lock free.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 5, 2016

Member

Updated to remove dead code I inadvertently left in rpcblockchain.cpp.

Member

sdaftuar commented Jan 5, 2016

Updated to remove dead code I inadvertently left in rpcblockchain.cpp.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 6, 2016

Member

Here's an example of a tool that would benefit from the getdescendants function:
https://github.com/sdaftuar/replace-by-fee-tools/blob/d0121b03d02c1a7e5e16425b55da1302ddf081aa/combine-transactions.py

This is a script that takes two txid's (that are signaling opt-in RBF) and combines them down to one -- consolidating duplicate output addresses (if any) and removing pay-to-self outputs, assumed for demo purposes to be change outputs -- and paying for all fees including those of descendants. The main idea is that this avoids over-paying for descendants -- it calls getdescendants on each transaction and takes the union of the two sets so that each descendant is paid for once. I don't know any simple or straightforward way to do this without this new RPC call.

There are similar benefits to having getancestors, such as if you wanted to understand why a stuck transaction wasn't confirming, then getancestors would give you the set of unconfirmed parents to look at (and you could write a replace-by-fee-tool that was smart enough to drop an input coming from a stuck unconfirmed parent, when possible, to replace a transaction with one likely to confirm sooner; or better still, replace such a stuck parent instead if possible).

Member

sdaftuar commented Jan 6, 2016

Here's an example of a tool that would benefit from the getdescendants function:
https://github.com/sdaftuar/replace-by-fee-tools/blob/d0121b03d02c1a7e5e16425b55da1302ddf081aa/combine-transactions.py

This is a script that takes two txid's (that are signaling opt-in RBF) and combines them down to one -- consolidating duplicate output addresses (if any) and removing pay-to-self outputs, assumed for demo purposes to be change outputs -- and paying for all fees including those of descendants. The main idea is that this avoids over-paying for descendants -- it calls getdescendants on each transaction and takes the union of the two sets so that each descendant is paid for once. I don't know any simple or straightforward way to do this without this new RPC call.

There are similar benefits to having getancestors, such as if you wanted to understand why a stuck transaction wasn't confirming, then getancestors would give you the set of unconfirmed parents to look at (and you could write a replace-by-fee-tool that was smart enough to drop an input coming from a stuck unconfirmed parent, when possible, to replace a transaction with one likely to confirm sooner; or better still, replace such a stuck parent instead if possible).

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jan 6, 2016

Member
Member

sipa commented Jan 6, 2016

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 7, 2016

Member

@sipa True. Perhaps getmempoolancestors/getmempooldescendants? I'm open to suggestions.

@laanwj if this code is ok I think it would make sense to include this in 0.12

Member

sdaftuar commented Jan 7, 2016

@sipa True. Perhaps getmempoolancestors/getmempooldescendants? I'm open to suggestions.

@laanwj if this code is ok I think it would make sense to include this in 0.12

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 12, 2016

Member

Updated with new RPC names (getmempoolancestors and getmempooldescendants).

Member

sdaftuar commented Jan 12, 2016

Updated with new RPC names (getmempoolancestors and getmempooldescendants).

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jan 15, 2016

Member

Any reason not to just add the ancestors/descendants info to getrawmempool (and also the getmempoolentry)?

Member

luke-jr commented Jan 15, 2016

Any reason not to just add the ancestors/descendants info to getrawmempool (and also the getmempoolentry)?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 15, 2016

Member

The goal was to avoid having to dump the entire mempool if you were only interested in a very small subset of it. But I suppose there's no reason we couldn't also show the txid's of ancestors and descendants when you call getrawmempool (with verbose=true). If that seems useful, I'm happy to add that.

(edited)
Oh, right, you are suggesting that, and then yes I agree it would also make sense to update getmempoolentry to match. Sure, I'll go ahead and do that.

Member

sdaftuar commented Jan 15, 2016

The goal was to avoid having to dump the entire mempool if you were only interested in a very small subset of it. But I suppose there's no reason we couldn't also show the txid's of ancestors and descendants when you call getrawmempool (with verbose=true). If that seems useful, I'm happy to add that.

(edited)
Oh, right, you are suggesting that, and then yes I agree it would also make sense to update getmempoolentry to match. Sure, I'll go ahead and do that.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 18, 2016

Member

@luke-jr On further thought, the downside to that is that doing the calculation could cause getrawmempool to get up to ~25x slower (default policy allows length 25 ancestor and descendant chains, and the calculation of ancestors and descendants requires walking those chains), which on a full mempool could be a meaningful -- and perhaps unacceptable? -- slowdown.

The other possibility would be to add a new option to getrawmempool to determine whether to do the calculation or not, but I think we try to avoid adding new arguments to existing RPC calls? So now I think it's probably better to leave as-is. Let me know if you disagree...

Member

sdaftuar commented Jan 18, 2016

@luke-jr On further thought, the downside to that is that doing the calculation could cause getrawmempool to get up to ~25x slower (default policy allows length 25 ancestor and descendant chains, and the calculation of ancestors and descendants requires walking those chains), which on a full mempool could be a meaningful -- and perhaps unacceptable? -- slowdown.

The other possibility would be to add a new option to getrawmempool to determine whether to do the calculation or not, but I think we try to avoid adding new arguments to existing RPC calls? So now I think it's probably better to leave as-is. Let me know if you disagree...

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 22, 2016

Member

Concept ACK

Member

laanwj commented Jan 22, 2016

Concept ACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 3, 2016

Member

Needs rebase.

Member

laanwj commented Feb 3, 2016

Needs rebase.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Feb 11, 2016

Member

Rebased. There weren't any actual conflicts though, so not sure why github thought it wouldn't merge cleanly?

Member

sdaftuar commented Feb 11, 2016

Rebased. There weren't any actual conflicts though, so not sure why github thought it wouldn't merge cleanly?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 12, 2016

Member

Thanks!
I've noticed that before. Github is somewhat less good (or "more careful") at merging than git.

Member

laanwj commented Feb 12, 2016

Thanks!
I've noticed that before. Github is somewhat less good (or "more careful") at merging than git.

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016

Refactor logic for converting mempool entries to JSON
Github-Pull: #7292
Rebased-From: bf4fbe814df537e69d5b1092565efb6f8bb618b9

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016

Add getmempoolancestors RPC call
Github-Pull: #7292
Rebased-From: 2409317c8ba1ea11b0bb519121f8c441439a71db

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016

Add getmempooldescendants RPC call
Github-Pull: #7292
Rebased-From: 9644106526eea536cf056b4058a40b0dbbdbe32f

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016

Add getmempoolentry RPC call
Github-Pull: #7292
Rebased-From: 31ef9d5c649beea8812d2ba92f779d639e1c8b2c

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016

Add test coverage for new RPC calls
Github-Pull: #7292
Rebased-From: bc9020c1ab30ea9527ef63d4df4df10513f8756f
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 17, 2016

Member

Rebase needed.

Member

sipa commented May 17, 2016

Rebase needed.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar May 17, 2016

Member

Rebased. Also added a commit that extends the mempool entry output to include the ancestor state (added by #7594), as well as a brief update to the release notes.

Member

sdaftuar commented May 17, 2016

Rebased. Also added a commit that extends the mempool entry output to include the ancestor state (added by #7594), as well as a brief update to the release notes.

@jonasschnelli

View changes

Show outdated Hide outdated src/rpc/blockchain.cpp
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli May 17, 2016

Member

I don't know why Travis WIN32 fails on the unit tests (sadly, It seems that the unit-test log is missing in the travis dump).

mempool_packages.py needs to be added to the rpc-tests.py script.

Tested ACK 69c9553586befdbc9ca6a474ae294bebe998163b modulo nits.

Member

jonasschnelli commented May 17, 2016

I don't know why Travis WIN32 fails on the unit tests (sadly, It seems that the unit-test log is missing in the travis dump).

mempool_packages.py needs to be added to the rpc-tests.py script.

Tested ACK 69c9553586befdbc9ca6a474ae294bebe998163b modulo nits.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar May 17, 2016

Member

Fixed @jonasschnelli's nits. No idea how these changes could affect the unit tests on any platform; the travis failure seems disturbing...

Member

sdaftuar commented May 17, 2016

Fixed @jonasschnelli's nits. No idea how these changes could affect the unit tests on any platform; the travis failure seems disturbing...

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 17, 2016

Contributor

ACK 170bfea

Contributor

paveljanik commented May 17, 2016

ACK 170bfea

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar May 18, 2016

Member

Pushed a fix to address @paveljanik's nit

Member

sdaftuar commented May 18, 2016

Pushed a fix to address @paveljanik's nit

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar May 18, 2016

Member

Unit tests are now failing on linux. I'll see if I can reproduce locally.

Member

sdaftuar commented May 18, 2016

Unit tests are now failing on linux. I'll see if I can reproduce locally.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar May 18, 2016

Member

Oh, I suppose it could be this: #8069 (comment)

Member

sdaftuar commented May 18, 2016

Oh, I suppose it could be this: #8069 (comment)

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar May 20, 2016

Member

Looks like the travis issue was fixed by #8070, so I think this is ready now.

Member

sdaftuar commented May 20, 2016

Looks like the travis issue was fixed by #8070, so I think this is ready now.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 20, 2016

Contributor

reACK 5379307

Contributor

paveljanik commented May 20, 2016

reACK 5379307

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke May 20, 2016

Member

utACK 5379307

Member

MarcoFalke commented May 20, 2016

utACK 5379307

@arowser

This comment has been minimized.

Show comment
Hide comment
@arowser

arowser May 25, 2016

Contributor

Can one of the admins verify this patch?

Contributor

arowser commented May 25, 2016

Can one of the admins verify this patch?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 3, 2016

Member

utACK 5379307a23177c33b5a869513d1d1d09790b0269

Member

sipa commented Jun 3, 2016

utACK 5379307a23177c33b5a869513d1d1d09790b0269

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jun 9, 2016

Member

Is there anything holding this up?

Member

sdaftuar commented Jun 9, 2016

Is there anything holding this up?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 9, 2016

Member

Can you squash the typo?

Member

sipa commented Jun 9, 2016

Can you squash the typo?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jun 9, 2016

Member

@sipa done

Member

sdaftuar commented Jun 9, 2016

@sipa done

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 9, 2016

Member

utACK 176e19b (same tree as before)

Member

sipa commented Jun 9, 2016

utACK 176e19b (same tree as before)

@sipa sipa merged commit 176e19b into bitcoin:master Jun 9, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

sipa added a commit that referenced this pull request Jun 9, 2016

Merge #7292: [RPC] Expose ancestor/descendant information over RPC
176e19b Mention new RPC's in release notes (Suhas Daftuar)
7f6eda8 Add ancestor statistics to mempool entry RPC output (Suhas Daftuar)
a9b8390 Add test coverage for new RPC calls (Suhas Daftuar)
b09b813 Add getmempoolentry RPC call (Suhas Daftuar)
0dfd869 Add getmempooldescendants RPC call (Suhas Daftuar)
8f7b5dc Add getmempoolancestors RPC call (Suhas Daftuar)
5ec0cde Refactor logic for converting mempool entries to JSON (Suhas Daftuar)

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 28, 2016

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7292: [RPC] Expose ancestor/descendant information over RPC
176e19b Mention new RPC's in release notes (Suhas Daftuar)
7f6eda8 Add ancestor statistics to mempool entry RPC output (Suhas Daftuar)
a9b8390 Add test coverage for new RPC calls (Suhas Daftuar)
b09b813 Add getmempoolentry RPC call (Suhas Daftuar)
0dfd869 Add getmempooldescendants RPC call (Suhas Daftuar)
8f7b5dc Add getmempoolancestors RPC call (Suhas Daftuar)
5ec0cde Refactor logic for converting mempool entries to JSON (Suhas Daftuar)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7292: [RPC] Expose ancestor/descendant information over RPC
176e19b Mention new RPC's in release notes (Suhas Daftuar)
7f6eda8 Add ancestor statistics to mempool entry RPC output (Suhas Daftuar)
a9b8390 Add test coverage for new RPC calls (Suhas Daftuar)
b09b813 Add getmempoolentry RPC call (Suhas Daftuar)
0dfd869 Add getmempooldescendants RPC call (Suhas Daftuar)
8f7b5dc Add getmempoolancestors RPC call (Suhas Daftuar)
5ec0cde Refactor logic for converting mempool entries to JSON (Suhas Daftuar)

codablock added a commit to codablock/dash that referenced this pull request Dec 22, 2017

Merge #7292: [RPC] Expose ancestor/descendant information over RPC
176e19b Mention new RPC's in release notes (Suhas Daftuar)
7f6eda8 Add ancestor statistics to mempool entry RPC output (Suhas Daftuar)
a9b8390 Add test coverage for new RPC calls (Suhas Daftuar)
b09b813 Add getmempoolentry RPC call (Suhas Daftuar)
0dfd869 Add getmempooldescendants RPC call (Suhas Daftuar)
8f7b5dc Add getmempoolancestors RPC call (Suhas Daftuar)
5ec0cde Refactor logic for converting mempool entries to JSON (Suhas Daftuar)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment