[RPC] Expose ancestor/descendant information over RPC #7292

pull sdaftuar wants to merge 7 commits into bitcoin:master from sdaftuar:add-chain-info changing 4 files +258 −39
  1. sdaftuar commented at 9:12 pm on January 4, 2016: member

    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.

  2. btcdrak commented at 3:19 am on January 5, 2016: contributor
    Concept ACK
  3. jonasschnelli commented at 8:08 am on January 5, 2016: contributor
    Concept ACK. Some concrete use-cases would be nice. Nice to see the new RPC commands cs_main lock free.
  4. jonasschnelli added the label RPC on Jan 5, 2016
  5. sdaftuar force-pushed on Jan 5, 2016
  6. sdaftuar force-pushed on Jan 5, 2016
  7. sdaftuar commented at 2:28 pm on January 5, 2016: member
    Updated to remove dead code I inadvertently left in rpcblockchain.cpp.
  8. sdaftuar commented at 6:43 pm on January 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).

  9. sipa commented at 7:14 pm on January 6, 2016: member
    Ultra nit before reading the code: it’s not obvious from the RPC names that it’s about the mempool.
  10. sdaftuar commented at 1:20 pm on January 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
  11. sdaftuar force-pushed on Jan 12, 2016
  12. sdaftuar commented at 2:03 pm on January 12, 2016: member
    Updated with new RPC names (getmempoolancestors and getmempooldescendants).
  13. luke-jr commented at 5:38 pm on January 15, 2016: member
    Any reason not to just add the ancestors/descendants info to getrawmempool (and also the getmempoolentry)?
  14. sdaftuar commented at 5:48 pm on January 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.

  15. sdaftuar commented at 3:45 pm on January 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…

  16. laanwj commented at 3:55 pm on January 22, 2016: member
    Concept ACK
  17. laanwj commented at 11:34 am on February 3, 2016: member
    Needs rebase.
  18. sdaftuar force-pushed on Feb 11, 2016
  19. sdaftuar commented at 8:47 pm on February 11, 2016: member
    Rebased. There weren’t any actual conflicts though, so not sure why github thought it wouldn’t merge cleanly?
  20. laanwj commented at 4:05 pm on February 12, 2016: member
    Thanks! I’ve noticed that before. Github is somewhat less good (or “more careful”) at merging than git.
  21. luke-jr referenced this in commit 85b0e45a09 on Feb 13, 2016
  22. luke-jr referenced this in commit 7ce4ee63ef on Feb 13, 2016
  23. luke-jr referenced this in commit dc7641d008 on Feb 13, 2016
  24. luke-jr referenced this in commit 9d557ea579 on Feb 13, 2016
  25. luke-jr referenced this in commit 24c954c4d4 on Feb 13, 2016
  26. sipa commented at 6:41 am on May 17, 2016: member
    Rebase needed.
  27. Refactor logic for converting mempool entries to JSON 5ec0cde371
  28. sdaftuar force-pushed on May 17, 2016
  29. sdaftuar commented at 1:07 pm on 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.
  30. in src/rpc/blockchain.cpp: in 69c9553586 outdated 
    371+            "\nArguments:\n"
372+            "1. \"txid\"                   (string, required) The transaction id (must be in mempool)\n"
373+            "2. verbose                  (boolean, optional, default=false) true for a json object, false for array of transaction ids\n"
374+            "\nResult (for verbose=false):\n"
375+            "[                       (json array of string)\n"
376+            "  \"transactionid\"       (string) The transaction id of an in-mempool descendant transaction\n"
    jonasschnelli commented at 2:56 pm on May 17, 2016:
    nit: missing four whitespace before (
  31. jonasschnelli commented at 3:13 pm on May 17, 2016: contributor

    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.

  32. Add getmempoolancestors RPC call 8f7b5dc4af
  33. sdaftuar force-pushed on May 17, 2016
  34. sdaftuar commented at 5:15 pm on 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…
  35. in src/rpc/blockchain.cpp: in 8f7b5dc4af outdated 
    300+            "\nIf txid is in the mempool, returns all in-mempool ancestors.\n"
301+            "\nArguments:\n"
302+            "1. \"txid\"                   (string, required) The transaction id (must be in mempool)\n"
303+            "2. verbose                  (boolean, optional, default=false) true for a json object, false for array of transaction ids\n"
304+            "\nResult (for verbose=false):\n"
305+            "[                       (json array of string)\n"
    paveljanik commented at 6:01 pm on May 17, 2016:
    json array of string_s_
  37. sdaftuar commented at 2:32 pm on May 18, 2016: member
    Pushed a fix to address @paveljanik’s nit
  38. sdaftuar commented at 3:43 pm on May 18, 2016: member
    Unit tests are now failing on linux. I’ll see if I can reproduce locally.
  39. sdaftuar commented at 3:44 pm on May 18, 2016: member
    Oh, I suppose it could be this: #8069#issue-155434890
  40. sdaftuar commented at 7:23 am on May 20, 2016: member
    Looks like the travis issue was fixed by #8070, so I think this is ready now.
  42. MarcoFalke commented at 8:56 am on May 20, 2016: member
    utACK 5379307
  43. arowser commented at 8:45 am on May 25, 2016: contributor
    Can one of the admins verify this patch?
  44. sipa commented at 2:06 pm on June 3, 2016: member
    utACK 5379307a23177c33b5a869513d1d1d09790b0269
  45. sdaftuar commented at 1:21 pm on June 9, 2016: member
    Is there anything holding this up?
  46. sipa commented at 1:29 pm on June 9, 2016: member
    Can you squash the typo?
  47. Add getmempooldescendants RPC call 0dfd86956d
  48. Add getmempoolentry RPC call b09b8135ae
  49. Add test coverage for new RPC calls a9b8390222
  50. Add ancestor statistics to mempool entry RPC output 7f6eda8043
  51. Mention new RPC's in release notes 176e19b571
  52. sdaftuar force-pushed on Jun 9, 2016
  53. sdaftuar force-pushed on Jun 9, 2016
  54. sdaftuar commented at 1:58 pm on June 9, 2016: member
    @sipa done
  55. sipa commented at 2:03 pm on June 9, 2016: member
    utACK 176e19b571f722437043d2aa80a69ae21852c70d (same tree as before)
  56. sipa merged this on Jun 9, 2016
  57. sipa closed this on Jun 9, 2016
  58. sipa referenced this in commit 7ce9ac5c83 on Jun 9, 2016
  59. luke-jr referenced this in commit 3184dac4a0 on Jun 28, 2016
  60. luke-jr referenced this in commit 52f73cac60 on Jun 28, 2016
  61. luke-jr referenced this in commit b055e254ac on Jun 28, 2016
  62. luke-jr referenced this in commit 6a325a1691 on Jun 28, 2016
  63. codablock referenced this in commit f715ba0811 on Sep 16, 2017
  64. codablock referenced this in commit 564b225f16 on Sep 19, 2017
  65. codablock referenced this in commit b9a00f6294 on Dec 22, 2017
  66. andvgal referenced this in commit 75458cec64 on Jan 6, 2019
  67. MarcoFalke locked this on Sep 8, 2021


sdaftuar btcdrak jonasschnelli sipa luke-jr laanwj paveljanik MarcoFalke arowser

Labels
RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me