[rpc] Allow fetching tx directly from specified block in getrawtransaction #10275

pull kallewoof wants to merge 3 commits into bitcoin:master from kallewoof:gettx-with-blockhash changing 4 files +109 −60
  1. kallewoof commented at 8:53 am on April 25, 2017: member

    [Reviewer hint: use ?w=1 to avoid seeing a bunch of indentation changes.]

    Presuming a user knows the block hash of the block containing a given transaction, this PR allows them to fetch the raw transaction, even without -txindex. It also enables support for getting transactions that are in orphaned blocks.

    Note that supplying a block hash will override mempool and txindex support in GetTransaction. The rationale behind this is that a transaction may be in multiple places (orphaned blocks) and if the user supplies an explicit block hash it should be adhered to.

     0$ # a41.. is a tx inside an orphan block ..3c6f.. -- first try getting it normally
 1$ ./bitcoin-cli getrawtransaction a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79 1
 2error code: -5
 3error message:
 4No such mempool transaction. Use -txindex to enable blockchain transaction queries. Use gettransaction for wallet transactions.
 5$ # now try with block hash
 6$ ./bitcoin-cli getrawtransaction a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79 1 0000000000000000003c6fe479122bfa4a9187493937af1734e1e5cd9f198ec7
 7{
 8  "hex": "01000000014e7e81144e42f6d65550e59b715d470c9301fd7ac189[...]90488ac00000000",
 9  "inMainChain": false,
10  "txid": "a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79",
11  "hash": "a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79",
12  "size": 225,
13[...]
14}
15$ # another tx 6c66... in block 462000
16$ ./bitcoin-cli getrawtransaction 6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735 1 00000000000000000217f2c12922e321f6d4aa933ce88005a9a493c503054a40
17{
18  "hex": "0200000004d157[...]88acaf0c0700",
19  "inMainChain": true,
20  "txid": "6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735",
21  "hash": "6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735",
22  "size": 666,
23[...]
24}
25$
  2. fanquake added the label RPC/REST/ZMQ on Apr 25, 2017
  3. in src/rpc/rawtransaction.cpp:207 in 221e725814 outdated 
    203 
204-    uint256 hash = ParseHashV(request.params[0], "parameter 1");
205+    uint256 hash;
206+    int64_t blockHeight = -1;
207+    bool inMainChain = true;
208+    auto p = request.params[0].get_str().find(':');
    laanwj commented at 11:47 am on April 25, 2017:

    This functionality could come in useful.

    As for the API I prefer to not do any string combining/parsing here, this makes the API less clean to work with at least in my experience. I’d prefer to add an optional (can be null or missing) fromblock argument.

  4. gmaxwell commented at 4:41 pm on April 25, 2017: contributor

    Concept ACK. I’ve really wanted this before.

    Allowing it work on orphan blocks is an interesting idea. I’m a little less sure about that– I think it could allow it to return transactions that have never been validated, which would be somewhat surprising.

  5. jonasschnelli commented at 7:37 pm on April 25, 2017: contributor
    Nice feature! I agree with @laanwj about the string parsing. The <block>:<txid> schematics looks good at first sight, but we are using JSON and should stay with JSON and don’t add another form or key/value encoding.
  6. kallewoof commented at 4:24 am on April 26, 2017: member
    Fair enough, I’ll add a blockhash argument instead. I was kind of toying with the idea of a new standard for referencing transactions which included the block height (not hash) so everyone could always find a tx presuming they had the block in question, and that thought sort of seeped in here.
  7. kallewoof commented at 5:12 am on April 26, 2017: member

    @gmaxwell

    Allowing it work on orphan blocks is an interesting idea. I’m a little less sure about that– I think it could allow it to return transactions that have never been validated, which would be somewhat surprising.

    I just realized my logic was flawed on this. I am passing the block height only to the GetTransaction method, which means it will always pick the active chain at the given height. Either I throw when the block is not in the main chain (i.e. no support for orphaned blocks) or I move the height determine logic over to validation.cpp. I am leaning towards the latter, but feedback welcome.

    Edit: passing CBlockIndex to GetTransaction seems like a great way to do this. Going with that.

  8. kallewoof force-pushed on Apr 26, 2017
  9. kallewoof commented at 5:44 am on April 26, 2017: member

    The code now works as advertised (see updated OP).

    History:

  10. kallewoof force-pushed on Apr 26, 2017
  11. kallewoof force-pushed on Apr 26, 2017
  12. kallewoof force-pushed on Apr 28, 2017
  13. kallewoof force-pushed on May 1, 2017
  14. in src/rpc/rawtransaction.cpp:213 in 23be83e143 outdated 
    211         if (request.params[1].isNum()) {
212-            if (request.params[1].get_int() != 0) {
213-                fVerbose = true;
214-            }
215+            fVerbose = (request.params[1].get_int() != 0);
216+        } else if (request.params[1].isBool()) {
    jnewbery commented at 9:26 pm on May 2, 2017:

    nit: You can replace this and the next three lines with:

    0} else {
1    fVerbose = (request.params[1].get_bool());
2}

    since get_bool() does the type testing for you and throws the JSONRPCError if the type isn’t a VBOOL.

    Up to you whether you think that’s clearer.

    kallewoof commented at 1:23 pm on May 4, 2017:
    Ohh, good point! Thanks. Edit: I don’t want it to throw for null values though.
    kallewoof commented at 1:26 pm on May 4, 2017:

    So I end up with

    0        if (request.params[1].isNum()) {
1            fVerbose = (request.params[1].get_int() != 0);
2        } else if (!request.params[1].isNull()) {
3            fVerbose = (request.params[1].get_bool());
4        }
  15. jnewbery commented at 9:27 pm on May 2, 2017: member
    utACK, but I think this deserves a new functional test case.
  16. kallewoof force-pushed on May 4, 2017
  17. kallewoof commented at 1:31 pm on May 4, 2017: member

    @jnewbery I agree. Will get to work on that.

    […]:

  18. kallewoof force-pushed on May 6, 2017
  19. kallewoof commented at 4:28 am on May 8, 2017: member

    @jnewbery Added some tests to rawtransactions.py for the included blockhash variant (a6b8461).

    […]:

  20. in test/functional/rawtransactions.py:74 in a6b84618ad outdated 
    69+        ############################
70+
71+        # make a tx by sending then generate 2 blocks; block1 has the tx in it,
72+        # presumably
73+        tx = self.nodes[2].sendtoaddress(self.nodes[1].getnewaddress(), 1)
74+        [ block1, block2 ] = self.nodes[2].generate(2)
    jnewbery commented at 7:28 pm on May 15, 2017:

    nit: no need for brackets here. The following should do:

    block1, block2 = self.nodes[2].generate(2)

  21. in test/functional/rawtransactions.py:77 in a6b84618ad outdated 
    72+        # presumably
73+        tx = self.nodes[2].sendtoaddress(self.nodes[1].getnewaddress(), 1)
74+        [ block1, block2 ] = self.nodes[2].generate(2)
75+        self.sync_all()
76+        # We should be able to get the raw transaction by providing the correct block
77+        assert self.nodes[0].getrawtransaction(tx, True, block1)
    jnewbery commented at 7:30 pm on May 15, 2017:

    nit: I think it’s better to assert on the actual value here (ie verify that the getrawtransaction returned the correct transaction rather than returned anything). The following should do that:

    assert_equal(self.nodes[0].getrawtransaction(tx, True, block1)['txid'], tx)

    kallewoof commented at 5:12 am on May 16, 2017:
    @jnewbery Good points, thanks. Addressed!
  22. jnewbery commented at 7:31 pm on May 15, 2017: member
    tested ACK the integration test in a6b84618ad04dccc628aa4905fced4784a62895a with a couple of nits.
  23. sipa commented at 10:32 pm on May 15, 2017: member
    Is this still needed after #8704?
  24. kallewoof commented at 2:41 am on May 16, 2017: member
    @sipa The use cases are quite different I think. #8704 lets you see all transactions in a given block. This lets you grab a transaction directly from a block without indexing.
  25. sipa commented at 2:42 am on May 16, 2017: member
    @kallewoof #8704 does not require indexing either
  26. kallewoof commented at 2:56 am on May 16, 2017: member
    Yeah, sorry, I meant that this is a way to get a specific transaction if you know the block hash, whereas #8704 shows you all transactions in the entire block. You get the info, but you have to wade through stuff to get it.
  27. laanwj commented at 5:25 am on May 16, 2017: member

    In both cases the whole block has to be loaded from disk, and parsed, and searched linearly. The difference is whether the linear search step happens on the server or client.

    I think both #8704 and this can be useful, but have to agree there’s only superficial difference.

  28. maaku commented at 8:32 am on May 16, 2017: contributor

    The difference between the two is the serialization and transmission and parsing of ~5MB of JSON data vs a few hundred bytes of hex encoded data. That’s a 1,000x difference on the client side, and the same absolute improvement on the server – although as a multiplier it’d be less since as you note the server still has to parse the block from disk. That’s a nontrivial performance difference.

    (Also, this would have greatly helped me in the past so another +1 from me.)

  29. jnewbery commented at 1:31 pm on May 16, 2017: member
    Agree with @maaku - simply encoding the data into 5MB of json could be time-consuming, during which time the cs_main lock is held. Having a command to return just a single transaction from a block seems very useful.
  30. in src/rpc/rawtransaction.cpp:133 in 8f84e8c6e1 outdated 
    131+            "getrawtransaction \"txid\" ( verbose \"blockhash\" )\n"
132 
133             "\nNOTE: By default this function only works for mempool transactions. If the -txindex option is\n"
134-            "enabled, it also works for blockchain transactions.\n"
135+            "enabled, it also works for blockchain transactions. If the block hash is known, it can be provided\n"
136+            "for nodes without -txindex.\n"
    luke-jr commented at 10:11 pm on June 3, 2017:
    Should probably mention that it MUST be in that block in this case…
  31. in src/rpc/rawtransaction.cpp:223 in 8f84e8c6e1 outdated 
    224+    if (request.params.size() > 2 && !request.params[2].isNull()) {
225+        uint256 blockHash = ParseHashV(request.params[2], "parameter 3");
226+        if (!blockHash.IsNull()) {
227+            BlockMap::iterator it = mapBlockIndex.find(blockHash);
228+            if (it == mapBlockIndex.end()) {
229+                throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block hash not found in chain");
    luke-jr commented at 10:13 pm on June 3, 2017:
    Remove “in chain”. Maybe “in database”?
  32. in src/rpc/rawtransaction.cpp:150 in 8f84e8c6e1 outdated 
    145             "\nResult (if verbose is not set or set to false):\n"
146             "\"data\"      (string) The serialized, hex-encoded data for 'txid'\n"
147 
148             "\nResult (if verbose is set to true):\n"
149             "{\n"
150+            "  \"inMainChain\": b,       (bool) Whether transaction is in the main chain or not. Only visible when specifying block hash\n"
    luke-jr commented at 10:14 pm on June 3, 2017:
    Whether the block specified is in the main chain or not… This could be false with the tx being still in the main chain!
  33. luke-jr changes_requested
  34. luke-jr referenced this in commit e096099bf7 on Jun 3, 2017
  35. luke-jr referenced this in commit 99dd1fcaab on Jun 3, 2017
  36. luke-jr referenced this in commit 57cb225d97 on Jun 3, 2017
  37. luke-jr referenced this in commit c188d00513 on Jun 3, 2017
  38. luke-jr referenced this in commit 675c1fedad on Jun 3, 2017
  39. luke-jr commented at 10:28 pm on June 3, 2017: member
    Suggested message fix on my gettx-with-blockhash-0.14 branch (cherry-pick).
  40. sipa commented at 6:44 pm on June 4, 2017: member
    Needs rebase.
  41. kallewoof force-pushed on Jun 5, 2017
  42. kallewoof commented at 0:19 am on June 5, 2017: member

    @luke-jr Thanks for the review! I cherry-picked your commit.

    […]:

  43. kallewoof force-pushed on Jun 5, 2017
  44. in src/rpc/rawtransaction.cpp:154 in caa27c1d5d outdated 
    159-            if(request.params[1].isTrue()) {
160-                fVerbose = true;
161+    }
162+
163+    if (request.params.size() > 2 && !request.params[2].isNull()) {
164+        uint256 blockHash = ParseHashV(request.params[2], "parameter 3");
    TheBlueMatt commented at 6:57 pm on June 7, 2017:
    Hmm? Shouldn’t we use the parameter’s name here instead of “parameter 3”?
    kallewoof commented at 4:37 am on June 8, 2017:
    The general tendency seems to be to identify the parameter index so I stuck with that. I agree it may be better to be more descriptive though…
  45. in src/rpc/rawtransaction.cpp:89 in caa27c1d5d outdated 
    84             "\nResult (if verbose is not set or set to false):\n"
85             "\"data\"      (string) The serialized, hex-encoded data for 'txid'\n"
86 
87             "\nResult (if verbose is set to true):\n"
88             "{\n"
89+            "  \"inMainChain\": b,       (bool) Whether specified block is in the main chain or not\n"
    TheBlueMatt commented at 7:06 pm on June 7, 2017:
    Hmm, maybe say “if blockhash is specified” or otherwise mention this wont appear unless a blockhash is provided. Even better, fill it out if GetTransaction returns the blockhash cause it found it via UTXO/txindex.
    kallewoof commented at 4:36 am on June 8, 2017:

    I was sure I did, but guess not:

    0            "  \"inMainChain\": b,     (bool) Whether specified block is in the main chain or not (only present with explicit \"blockhash\" argument)\n"

    I like the idea of including when able but will keep it out of this PR for now.

  46. in src/rpc/rawtransaction.cpp:71 in caa27c1d5d outdated 
    69+            "getrawtransaction \"txid\" ( verbose \"blockhash\" )\n"
70 
71             "\nNOTE: By default this function only works for mempool transactions. If the -txindex option is\n"
72-            "enabled, it also works for blockchain transactions.\n"
73+            "enabled, it also works for blockchain transactions. If the block hash is known, it can be provided\n"
74+            "for nodes without -txindex, in which case the transaction will only be found if it is in that\n"
    TheBlueMatt commented at 7:12 pm on June 7, 2017:

    grammar nit: this reads funny to me, and could be a bit more explicit. Maybe:

    “If the block which contains the transaction is known, its hash can be provided even for nodes without -txindex.” “Note that if a blockhash is provided, only it will be searched and if the transaction is in mempool, other blocks, or if this node does not have the given block available, the transaction will not be found.”

    kallewoof commented at 4:34 am on June 8, 2017:

    Thanks, that looks better yeah. Adding with minor tweaks.

    0            "\nNOTE: By default this function only works for mempool transactions. If the -txindex option is\n"
1            "enabled, it also works for blockchain transactions. If the block which contains the transaction\n"
2            "is known, its hash can be provided even for nodes without -txindex. Note that if a blockhash is\n"
3            "provided, only that block will be searched and if the transaction is in the mempool or other\n"
4            "blocks, or if this node does not have the given block available, the transaction will not be found.\n"
  47. in src/rpc/rawtransaction.cpp:172 in caa27c1d5d outdated 
    178     CTransactionRef tx;
179     uint256 hashBlock;
180-    if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock, true))
181-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string(fTxIndex ? "No such mempool or blockchain transaction"
182+    if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock, true, blockIndex))
183+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string(fTxIndex || blockIndex ? "No such mempool or blockchain transaction"
    TheBlueMatt commented at 7:13 pm on June 7, 2017:
    Maybe further update this error message, eg if (blockIndex) “No such transaction found in the provided block”.
    kallewoof commented at 4:41 am on June 8, 2017:

    Yeah, I wanted to avoid ?:?:. Rewritten as:

     0    if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock, true, blockIndex)) {
 1        std::string errmsg;
 2        if (blockIndex) {
 3            errmsg = "No such transaction found in the provided block";
 4        } else {
 5            errmsg = fTxIndex
 6              ? "No such mempool or blockchain transaction"
 7              : "No such mempool transaction. Use -txindex to enable blockchain transaction queries";
 8        }
 9        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, errmsg + ". Use gettransaction for wallet transactions.");
10    }
  48. TheBlueMatt commented at 7:16 pm on June 7, 2017: member
    You may wish to squash “[rpc] Fix fVerbose parsing (remove excess if cases and ensure null is…” and “[test] Updated rawtransactions.py to assert for adjusted exception.”. Generally we try to make sure that after each individual commit, at least it builds and all tests pass.
  49. kallewoof commented at 4:43 am on June 8, 2017: member

    @TheBlueMatt Thanks for the review!

    Generally we try to make sure that after each individual commit, at least it builds and all tests pass.

    We try to keep tests as separate commits though, so that would assume tests and code changes come in pairs (tests will fail before test commit or after test commit and before change commit, obv). That was my intention with the split here. I may have screwed up. I’ll double check and/or squash as appropriate.

    Edit: I noticed the order was off (two fixes then two tests). Rearranged them. The commit/test pairs now pass make check individually (i.e. a43ec61 and 8f2ce52).

    Edit 2: […]:

  50. kallewoof force-pushed on Jun 8, 2017
  51. kallewoof force-pushed on Jun 8, 2017
  52. luke-jr referenced this in commit 5dfa87a5a4 on Jun 15, 2017
  53. luke-jr referenced this in commit f080d5f407 on Jun 15, 2017
  54. luke-jr referenced this in commit 633d3340dd on Jun 15, 2017
  55. luke-jr referenced this in commit c35815aaed on Jun 15, 2017
  56. TheBlueMatt commented at 9:15 pm on June 22, 2017: member
    utACK 8f2ce52c9257392d7e6523b39a26e9697a30ca76
  57. jonasschnelli commented at 8:09 am on July 14, 2017: contributor
    Needs rebase.
  58. kallewoof force-pushed on Jul 14, 2017
  59. kallewoof commented at 9:48 am on July 14, 2017: member
    Rebased.
  60. jonasschnelli commented at 9:53 am on July 14, 2017: contributor

    a) Is there a reason why mainchain height is not supported as alternative for the blockhash? Eventually with a security of only accepting heights of a hundred blocks below the tip as a reorganisation protection (but I’d prefer to not add this protection).

    b) @kallewoof the idea about the standard for a transaction reference has already been worked into a BIP: https://github.com/bitcoin/bips/pull/555 (maybe we can support this – if we agree on that BIP to be worth implementing – also via getrawtransaction).

  61. kallewoof commented at 10:18 am on July 14, 2017: member

    @jonasschnelli Regarding height, I chose not to include it as it could potentially cause unexpected results when a reorg happens, but if people don’t think that’s an issue it should be fairly straightforward to allow for both.

    Edit: as for the standard, that looks exciting for sure. If it matures enough and this PR isn’t merged already I may take a stab at it.

  62. in src/rpc/rawtransaction.cpp:89 in 3ec2d287e2 outdated 
    84             "\nResult (if verbose is not set or set to false):\n"
85             "\"data\"      (string) The serialized, hex-encoded data for 'txid'\n"
86 
87             "\nResult (if verbose is set to true):\n"
88             "{\n"
89+            "  \"inMainChain\": b,     (bool) Whether specified block is in the main chain or not (only present with explicit \"blockhash\" argument)\n"
    jtimon commented at 8:30 pm on July 17, 2017:
    This may be confused with Params().NetworkIDString() == “main” (kind of like we do with “testnet” in getinfo). Can we rename to “inActiveChain” or something of the sort?
  63. in src/validation.cpp:939 in 3ec2d287e2 outdated 
    912-    if (ptx)
913-    {
914-        txOut = ptx;
915-        return true;
916-    }
917+    if (!blockIndex) {
    jtimon commented at 8:38 pm on July 17, 2017:
    Perhaps the diff can be less disruptive by moving everything inside if (!blockIndex) {...} to a new static function defined right above instead of indenting all of it?
    kallewoof commented at 3:32 am on July 18, 2017:
    I’m not a huge fan of changing code just to make diffs smaller, and as I mention above, you can put ?w=1 to get diff without indentation changes.
  64. jtimon commented at 8:45 pm on July 17, 2017: contributor
    utACK 3ec2d287e24c9cc573ead2898546499abf49db9d besides small nits.
  65. kallewoof force-pushed on Jul 18, 2017
  66. kallewoof commented at 3:45 am on July 18, 2017: member
    Addressed @jtimon nits. @jonasschnelli I added support for supplying block height instead of block hash.
  67. jtimon commented at 9:59 pm on July 18, 2017: contributor
    I’m not sure about allowing the height. One can always call getblockhash if he knows the txid and block height without knowing the block hash somehow. That’s not the case for the BIP proposal @jonasschnelli refers to, since they don’t know the txid anyway (only block height and tx position). I suggest we leave the BIP discussion out of this PR, which in my opinion makes sense on its own. And also leave the height option for later (or never).
  68. kallewoof commented at 2:15 am on July 19, 2017: member
    @jtimon It feels like a convenience thing that, IMO, getblock should have as well (i.e. allow both height number and block hash). I don’t have a strong opinion on the subject though, and will drop the last 2 commits (height support) whenever this is ready for merging unless someone argues for block height support.
  69. jtimon commented at 7:46 pm on July 20, 2017: contributor
    Well, don’t have a strong opinion on adding the block height either, if more people like it, let’s keep it. Or perhaps it can be proposed after this for both getrawtransaction and getblock at the same time?
  70. instagibbs commented at 3:12 pm on August 23, 2017: member
    concept ACK, needs rebase/squash, would review
  71. kallewoof force-pushed on Aug 24, 2017
  72. kallewoof commented at 7:25 am on August 24, 2017: member
    @instagibbs Rebased & squashed; also removed block height option for now. Will push as separate PR if desirable.
  73. kallewoof force-pushed on Aug 24, 2017
  74. kallewoof force-pushed on Aug 24, 2017
  75. kallewoof force-pushed on Aug 24, 2017
  76. kallewoof force-pushed on Aug 24, 2017
  77. kallewoof force-pushed on Aug 24, 2017
  78. kallewoof force-pushed on Aug 24, 2017
  79. in src/rpc/rawtransaction.cpp:89 in cc75dab930 outdated 
    84             "\nResult (if verbose is not set or set to false):\n"
85             "\"data\"      (string) The serialized, hex-encoded data for 'txid'\n"
86 
87             "\nResult (if verbose is set to true):\n"
88             "{\n"
89+            "  \"inActiveChain\": b,   (bool) Whether specified block is in the active chain or not (only present with explicit \"blockhash\" argument)\n"
    instagibbs commented at 1:38 pm on August 24, 2017:
    maybe in_active_chain? I’m parsing this as “inactive chain”
  80. in src/rpc/rawtransaction.cpp:143 in cc75dab930 outdated 
    138+            + HelpExampleCli("getrawtransaction", "\"mytxid\" true \"myblockhash\"")
139         );
140 
141     LOCK(cs_main);
142 
143+    bool inActiveChain = true;
    instagibbs commented at 1:40 pm on August 24, 2017:
    in_active_chain to conform to new style guide and make easier to read.
  81. in src/validation.cpp:970 in cc75dab930 outdated 
    958-    }
959 
960-    if (fAllowSlow) { // use coin database to locate block that contains transaction, and scan it
961-        const Coin& coin = AccessByTxid(*pcoinsTip, hash);
962-        if (!coin.IsSpent()) pindexSlow = chainActive[coin.nHeight];
963+        if (fAllowSlow) { // use coin database to locate block that contains transaction, and scan it
    instagibbs commented at 1:47 pm on August 24, 2017:
    should skip this logic if user has provided the blockhash?
    kallewoof commented at 1:09 am on August 25, 2017:
    This is inside the if (!blockIndex) block so it should never execute if a block hash was provided, no?
    instagibbs commented at 1:27 am on August 25, 2017:
    Oops! Not good at counting brackets.
  82. instagibbs changes_requested
  83. instagibbs commented at 1:52 pm on August 24, 2017: member

    Current behavior of GetTransaction means that even if you specify a block index, it still does all the other searches. This means that a user could specify an incorrect blockhash, and still return a valid result, with “inActive” == False(though it may actually be in the valid chain!).

    As a user I would expect any call to fail if I simply gave the wrong blockhash, but that’s simply my opinion.

  84. kallewoof force-pushed on Aug 25, 2017
  85. instagibbs commented at 1:29 am on August 25, 2017: member

    I fail at counting brackets.

    utACK https://github.com/bitcoin/bitcoin/pull/10275/commits/440123fb8c7196a7ee209495f02b2f7c0deaa0bb

  86. in src/rpc/rawtransaction.cpp:92 in 440123fb8c outdated 
    84             "\nResult (if verbose is not set or set to false):\n"
85             "\"data\"      (string) The serialized, hex-encoded data for 'txid'\n"
86 
87             "\nResult (if verbose is set to true):\n"
88             "{\n"
89+            "  \"in_active_chain\": b, (bool) Whether specified block is in the active chain or not (only present with explicit \"blockhash\" argument)\n"
    jnewbery commented at 2:32 pm on September 1, 2017:
    nit: alignment
    kallewoof commented at 2:15 am on September 4, 2017: 
    0            "  \"in_active_chain\": b, (bool) Whether specified block is in the active chain or not (only present with explicit \"blockhash\" argument)\n"
1            "  \"hex\" : \"data\",       (string) The serialized, hex-encoded data for 'txid'\n"

    becomes

    0  "in_active_chain": b, (bool) Whether specified block is in the active chain or not (only present with explicit "blockhash" argument)
1  "hex" : "data",       (string) The serialized, hex-encoded data for 'txid'

    (two \ vs four \ means indentation diff in code)

    jnewbery commented at 5:03 pm on September 4, 2017:
    Yes, of course you’re correct. The size, vsize, version, … fields below aren’t aligned by the same logic, but that’s beyond the scope of this PR. Please disregard my previous comment.
  87. in src/rpc/rawtransaction.cpp:176 in 440123fb8c outdated 
    181-            ". Use gettransaction for wallet transactions.");
182+    uint256 hash_block;
183+    if (!GetTransaction(hash, tx, Params().GetConsensus(), hash_block, true, blockindex)) {
184+        std::string errmsg;
185+        if (blockindex) {
186+            errmsg = "No such transaction found in the provided block";
    jnewbery commented at 2:34 pm on September 1, 2017:

    I think we would also hit this error if the block had been pruned. In that case, this error is misleading.

    Is there any way to test above for whether the block has been pruned and error out before doing the transaction lookup?

    kallewoof commented at 2:31 am on September 4, 2017:

    Good point!

    I added code (https://github.com/bitcoin/bitcoin/pull/10275/commits/3c0cb434bde411d2daf8e4a440c85e80b1eb1f62) that checks if pruning is enabled, and if it is, checks if the block is actually available by reading it from disk and setting the error message based on the results of that.

  88. in test/functional/rawtransactions.py:58 in 440123fb8c outdated 
    51@@ -52,6 +52,20 @@ def run_test(self):
52         # This will raise an exception since there are missing inputs
53         assert_raises_jsonrpc(-25, "Missing inputs", self.nodes[2].sendrawtransaction, rawtx['hex'])
54 
55+        ############################
56+        # getrawtx with block hash #
57+        ############################
58+
59+        # make a tx by sending then generate 2 blocks; block1 has the tx in it,
60+        # presumably
    jnewbery commented at 2:43 pm on September 1, 2017:
    Lose the presumably :)
  89. in test/functional/rawtransactions.py:65 in 440123fb8c outdated 
    62+        block1, block2 = self.nodes[2].generate(2)
63+        self.sync_all()
64+        # We should be able to get the raw transaction by providing the correct block
65+        assert_equal(self.nodes[0].getrawtransaction(tx, True, block1)['txid'], tx)
66+        # We should not get the tx if we provide an unrelated block
67+        assert_raises_jsonrpc(-5, "No such", self.nodes[0].getrawtransaction, tx, True, block2)
    jnewbery commented at 2:44 pm on September 1, 2017:
    You need to make the matching error text longer to be non-ambiguous. This test will still pass if the node returns the error No such mempool or blockchain transaction (which would be incorrect behaviour)
    kallewoof commented at 2:34 am on September 4, 2017:
    Oops. Fixed, thanks.
  90. jnewbery commented at 3:44 pm on September 1, 2017: member

    A few nits inline.

    I think you need to squash the first two commits to make the code and test changes atomic (otherwise the test fails on the intermediate 90e140862ac7ac172cf89ed88e8183984affe057 commit)

  91. kallewoof force-pushed on Sep 4, 2017
  92. kallewoof commented at 2:37 am on September 4, 2017: member

    @jnewbery Thanks for the review! I think I’ve addressed all your comments.

    Edit: Actually I forgot about the commit merge. I try to keep tests and code changes as separate commits. Why would you want it to succeed between the test update? (Or am I confused about what you’re asking?)

  93. jnewbery commented at 5:07 pm on September 4, 2017: member

    I try to keep tests and code changes as separate commits. Why would you want it to succeed between the test update? (Or am I confused about what you’re asking?)

    I think the principal is that each individual commit should be at least internally consistent (should build and pass its own tests). Your first commit updates the product code so the returned error changes, and your second commit updates the test code to pass with the new error string. That means that if I run the tests after the first commit they’ll fail.

    I understand the desire to structure the commits in a way that tells a story, but this breaks tools like git bisect that rely on commits to be internally consistent.

  94. kallewoof commented at 1:27 am on September 5, 2017: member
    @jnewbery You can git bisect skip if you land on a commit squeezed in between tests. I don’t really agree with squashing test updates and code changes together, personally. That’s how I’ve always done it, anyway.
  95. promag commented at 7:14 am on September 5, 2017: member
    I agree with @jnewbery, one commit should not break the code, tests including. If you revert that commit, codes stays green. IMO makes reviewing easier.
  96. in src/rpc/rawtransaction.cpp:171 in 67caecc0ae outdated 
    182+    uint256 hash_block;
183+    if (!GetTransaction(hash, tx, Params().GetConsensus(), hash_block, true, blockindex)) {
184+        std::string errmsg;
185+        if (blockindex) {
186+            bool block_available = true;
187+            if (gArgs.GetArg("-prune", 0)) {
    luke-jr commented at 8:28 pm on September 5, 2017:
    Use fHavePruned
  97. in src/rpc/rawtransaction.cpp:174 in 67caecc0ae outdated 
    185+        if (blockindex) {
186+            bool block_available = true;
187+            if (gArgs.GetArg("-prune", 0)) {
188+                // we are pruning; see if block is available at all
189+                CBlock block;
190+                block_available = ReadBlockFromDisk(block, blockindex, Params().GetConsensus());
    luke-jr commented at 8:31 pm on September 5, 2017:
    Use !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0 (see getblock)
  98. luke-jr changes_requested
  99. jtimon commented at 10:30 pm on September 5, 2017: contributor
    I agree individual commits should never break the tests ideally. That doesn’t mean that you can’t introduce new functionality and its new tests in separated commits when that’s not the case. Needs rebase again.
  100. jnewbery commented at 11:20 pm on September 5, 2017: member

    Last two commits look good. For the pruning error, I think you can error out earlier by re-using the pruned test from getblock:

    0     if (fHavePruned && !(pblockindex->nStatus & BLOCK_HAVE_DATA) && pblockindex->nTx > 0)
1        throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)");

    I don’t really agree with squashing test updates and code changes together, personally.

    ok. I think both ways are valid, and I’ve come across both before. I’m not sure if this project has a preferred style.

  101. kallewoof commented at 2:52 am on September 6, 2017: member
    I’m merging those commits as it’s not new functionality but just a fix that changes an error message.
  102. kallewoof commented at 2:53 am on September 6, 2017: member
    @luke-jr Nice!
  103. kallewoof force-pushed on Sep 6, 2017
  104. kallewoof force-pushed on Sep 6, 2017
  105. in src/rpc/rawtransaction.cpp:173 in 3ff7a57071 outdated 
    184+    if (!GetTransaction(hash, tx, Params().GetConsensus(), hash_block, true, blockindex)) {
185+        std::string errmsg;
186+        if (blockindex) {
187+            bool block_pruned = fHavePruned && !(blockindex->nStatus & BLOCK_HAVE_DATA) && blockindex->nTx > 0;
188+            errmsg = block_pruned
189+              ? "Block not available (pruned data)"
    jnewbery commented at 8:06 pm on September 6, 2017:

    supernit: Not finding a block because it’s been pruned is not a RPC_INVALID_ADDRESS_OR_KEY error. It would more accurately be a RPC_MISC_ERROR.

    My suggestion would be to do the pruned check above, immediately after the throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block hash not found"); line

    kallewoof commented at 2:17 am on September 7, 2017:
    Yeah, I was thinking about that too. Fixing.
  106. kallewoof force-pushed on Sep 7, 2017
  107. jtimon commented at 10:05 am on September 23, 2017: contributor
    re-utACK 300a5f15d54484691d47eb5ec23acde6d82bfda1
  108. kallewoof force-pushed on Oct 11, 2017
  109. kallewoof force-pushed on Oct 11, 2017
  110. in src/rpc/rawtransaction.cpp:173 in 4d15dce560 outdated 
    182-            ". Use gettransaction for wallet transactions.");
183+    uint256 hash_block;
184+    if (!GetTransaction(hash, tx, Params().GetConsensus(), hash_block, true, blockindex)) {
185+        std::string errmsg;
186+        if (blockindex) {
187+            if (fHavePruned && !(blockindex->nStatus & BLOCK_HAVE_DATA) && blockindex->nTx > 0) {
    TheBlueMatt commented at 10:20 pm on November 27, 2017:
    Why check nTx here? Its just as useful to provide an error of “Block not available” if it has been pruned as if it has not yet been received in full.
    kallewoof commented at 0:51 am on November 28, 2017:

    This is how the check is done in a lot of places. E.g. https://github.com/kallewoof/bitcoin/blame/gettx-with-blockhash/src/rest.cpp#L219

    I’m not sure why an additional nTx check is needed, to be honest.

    sipa commented at 0:55 am on November 28, 2017:
    “nTx > 0” means we had the block’s contents at some point.
    TheBlueMatt commented at 1:42 am on November 28, 2017:
    Yes, I’m saying remove the fHavePruned and nTx check here - if we have a block’s header but not its body, then we’ll throw a “No such transaction found in the provided block” error, which is nonsense. Easier to just make this error generic.
    kallewoof commented at 1:47 am on November 28, 2017:
    Ahh, OK.
  111. TheBlueMatt commented at 10:21 pm on November 27, 2017: member
    Needs trivial rebase. Note that first commit message is no longer correct (says “ensure null is accepted” but null is already accepted on master, commit contents are still good, though). Otherwise largely looks good.
  112. kallewoof force-pushed on Nov 28, 2017
  113. kallewoof force-pushed on Nov 28, 2017
  114. kallewoof force-pushed on Nov 28, 2017
  115. [rpc] Fix fVerbose parsing (remove excess if cases). a5f5a2ce53
  116. kallewoof force-pushed on Nov 29, 2017
  117. kallewoof force-pushed on Nov 29, 2017
  118. in src/rpc/rawtransaction.cpp:174 in 9cdcd16253 outdated 
    185+    uint256 hash_block;
186+    if (!GetTransaction(hash, tx, Params().GetConsensus(), hash_block, true, blockindex)) {
187+        std::string errmsg;
188+        if (blockindex) {
189+            if (!(blockindex->nStatus & BLOCK_HAVE_DATA)) {
190+                throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)");
    TheBlueMatt commented at 4:31 pm on December 4, 2017:
    Now the error message is wrong :/. You should drop the “(pruned data)” note, as this can happen if you’ve received the header but not (yet) the block.
    kallewoof commented at 11:26 pm on December 4, 2017:
    Oh, damn. Sorry about that. Fixed.
  119. in src/validation.cpp:963 in 9cdcd16253 outdated 
    964+        if (ptx) {
965+            txOut = ptx;
966             return true;
967         }
968 
969-        // transaction not found in index, nothing more can be done
    TheBlueMatt commented at 4:36 pm on December 4, 2017:
    Looks like this line was lost in a rebase? Shouldn’t remove it.
    kallewoof commented at 11:27 pm on December 4, 2017:
    Fixed!
  120. kallewoof force-pushed on Dec 4, 2017
  121. sipa commented at 1:02 am on December 5, 2017: member

    I realize I’m very late with this, so this is just for consideration and not an objection to this PR.

    I believe getrawtransaction is already a weird mixed bag of semantics (it works when a transaction is confirmed but at least one output is not yet spent by a confirmed transaction, or if the transaction is unconfirmed, or if txindex is available for everything but the genesis block). Adding yet another combination that works in some cases will only further the difficulty of explaining it.

    Ideally, I think this RPC should be replaced with a lookuprawtransaction (or so) which only works when -txindex is enabled. In case you only know a transaction is unconfirmed, use getmempoolentry which already exists. This functionality provided here should IMHO just be a separate RPC.

  122. in src/rpc/rawtransaction.cpp:164 in def73636a0 outdated 
    170             }
171-        }
172-        else {
173-            throw JSONRPCError(RPC_TYPE_ERROR, "Invalid type provided. Verbose parameter must be a boolean.");
174+            blockindex = it->second;
175+            in_active_chain = (chainActive[blockindex->nHeight] == blockindex);
    promag commented at 1:14 am on December 5, 2017:
    Use chainActive.Contains()?
  123. TheBlueMatt commented at 1:17 am on December 5, 2017: member

    Dear God, this PR is 7 months old. The behavior changes are all well-documented and it’s a very useful feature, there’s gotta be a statue of limitations on concept objections.

    On December 4, 2017 8:02:52 PM EST, Pieter Wuille notifications@github.com wrote:

    I realize I’m very late with this, so this is just for consideration and not an objection to this PR.

    I believe getrawtransaction is already a weird mixed bag of semantics (it works when a transaction is confirmed but at least one output is not yet spent by a confirmed transaction, or if the transaction is unconfirmed, or if txindex is available for everything but the genesis block). Adding yet another combination that works will only further the difficulty of explaining it.

    Ideally, I think this RPC should be replaced with a lookuprawtransaction (or so) which only works when -txindex is enabled. In case you only know a transaction is unconfirmed, use getmempoolentry which already exists. This functionality provided here should IMHO just be a separate RPC.

    – You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/10275#issuecomment-349160332

  124. in src/rpc/rawtransaction.cpp:157 in def73636a0 outdated 
    160-                fVerbose = true;
161+        fVerbose = request.params[1].isNum() ? (request.params[1].get_int() != 0) : request.params[1].get_bool();
162+    }
163+
164+    if (!request.params[2].isNull()) {
165+        uint256 blockhash = ParseHashV(request.params[2], "parameter 3");
    promag commented at 1:18 am on December 5, 2017:

    Add tests for invalid hash?

    • must be string;
    • must be hexadecimal string;
    • must be of length.
    promag commented at 2:21 pm on December 5, 2017:
    Could use parameter name instead of parameter 3? This is ugly for named args. (fix others in a new commit or in a follow up PR, whatever).
    kallewoof commented at 3:27 am on December 6, 2017:
    This is used in a ton of places. I never saw consensus on what exactly to do and chose to stick with it for now. As you said, different PR.
    jnewbery commented at 10:22 pm on December 6, 2017:

    Nit: these lines are confusing for me:

    0        uint256 blockhash = ParseHashV(request.params[2], "parameter 3");
1        if (!blockhash.IsNull()) {

    ParseHashV() can either throw or return a uint256. The only way it’ll return a null uint256 is if the input is a 64 length string of 0s. That can’t be a valid block (if someone finds a block that hashes to zero then they win Bitcoin The Game and we can all stop playing), so I think that this if condition is unnecessary - if parameter 3 is all zeroes, we should throw with the error that 0x0 is an invalid block.

    I don’t think this is a huge problem - if all zeroes is supplied as the block hash, we’ll just drop through and search for the transaction in the {mempool , blockchain (if fTxIndex) , UTXO set} as if no block hash had been provided. However, it’s a bit confusing for the code reader since it implies to me that we’re expecting ParseHashV() to return a null value if it fails to parse the hash.

  125. in src/rpc/rawtransaction.cpp:190 in def73636a0 outdated 
    203         return EncodeHexTx(*tx, RPCSerializationFlags());
204+    }
205 
206     UniValue result(UniValue::VOBJ);
207-    TxToJSON(*tx, hashBlock, result);
208+    if (blockindex) result.push_back(Pair("in_active_chain", in_active_chain));
    promag commented at 1:22 am on December 5, 2017:
    Missing test for in_active_chain.
  126. sipa commented at 1:28 am on December 5, 2017: member
    I’m explicitly not objecting in any way, just offering a view to consider.
  127. kallewoof commented at 1:31 am on December 5, 2017: member

    I agree getrawtransaction is very confusing until you understand its limitations. I’ll gladly work on streamlining it into potentially two or more RPC calls post-merge. Biggest concern is backwards compatibility and (alternatively) bloat.

    Edit: I do think the confusion will be decreased when users are given the option of providing a block hash, as there was no way to get the tx before this PR.

  128. in test/functional/rawtransactions.py:54 in def73636a0 outdated 
    49@@ -50,6 +50,19 @@ def run_test(self):
50         # This will raise an exception since there are missing inputs
51         assert_raises_rpc_error(-25, "Missing inputs", self.nodes[2].sendrawtransaction, rawtx['hex'])
52 
53+        ############################
54+        # getrawtx with block hash #
    promag commented at 1:31 am on December 5, 2017:
    # getrawtransaction with block hash #?
  129. in src/validation.cpp:929 in def73636a0 outdated 
    926@@ -927,46 +927,47 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
927 }
928 
929 /** Return transaction in txOut, and if it was found inside a block, its hash is placed in hashBlock */
    promag commented at 1:31 am on December 5, 2017:
    Care to explain the new argument?
  130. in src/rpc/rawtransaction.cpp:84 in def73636a0 outdated 
    81@@ -79,12 +82,14 @@ UniValue getrawtransaction(const JSONRPCRequest& request)
82             "\nArguments:\n"
83             "1. \"txid\"      (string, required) The transaction id\n"
84             "2. verbose       (bool, optional, default=false) If false, return a string, otherwise return a json object\n"
    promag commented at 1:35 am on December 5, 2017:
    Nit, mind to fix verbose alignment?
  131. promag commented at 1:43 am on December 5, 2017: member
    IMO overloaded API’s are harder to maintain, test and document. Anyway, some comments below.
  132. [rpc] Allow getrawtransaction to take optional blockhash to fetch transaction from a block directly. b167951677
  133. kallewoof force-pushed on Dec 5, 2017
  134. kallewoof force-pushed on Dec 5, 2017
  135. kallewoof commented at 2:05 am on December 5, 2017: member
    @promag Thanks for the review! I think I addressed all your comments.
  136. in test/functional/rawtransactions.py:73 in 650e4eee92 outdated 
    64+        assert_equal(gottx['in_active_chain'], True)
65+        # We should not get the tx if we provide an unrelated block
66+        assert_raises_rpc_error(-5, "No such transaction found", self.nodes[0].getrawtransaction, tx, True, block2)
67+        # An invalid block hash should raise the correct errors
68+        assert_raises_rpc_error(-8, "parameter 3 must be hexadecimal", self.nodes[0].getrawtransaction, tx, True, True)
69+        assert_raises_rpc_error(-8, "parameter 3 must be hexadecimal", self.nodes[0].getrawtransaction, tx, True, "foobar")
    promag commented at 2:18 am on December 5, 2017:
    Can remove this.
    kallewoof commented at 3:26 am on December 5, 2017:
    First test checks for non-string, this one for non-hexadecimal string. May be redundant but they check different things.
  137. promag commented at 2:24 am on December 5, 2017: member
    Missing a test checking that in_active_chain is not in the response when blockhash is not provided.
  138. [test] Add tests for getrawtransaction with block hash. 434526aba6
  139. kallewoof force-pushed on Dec 5, 2017
  140. kallewoof commented at 3:28 am on December 5, 2017: member
    @promag Added test for unset in_active_chain.
  141. in test/functional/rawtransactions.py:215 in 434526aba6 
    211@@ -188,13 +212,13 @@ def run_test(self):
212         assert_equal(self.nodes[0].getrawtransaction(txHash, True)["hex"], rawTxSigned['hex'])
213 
214         # 6. invalid parameters - supply txid and string "Flase"
215-        assert_raises_rpc_error(-3,"Invalid type", self.nodes[0].getrawtransaction, txHash, "Flase")
216+        assert_raises_rpc_error(-1,"not a boolean", self.nodes[0].getrawtransaction, txHash, "Flase")
    promag commented at 2:15 pm on December 5, 2017:
    Nit, add space after ,. Same below.
  142. in test/functional/rawtransactions.py:61 in 434526aba6 
    56+
57+        # make a tx by sending then generate 2 blocks; block1 has the tx in it
58+        tx = self.nodes[2].sendtoaddress(self.nodes[1].getnewaddress(), 1)
59+        block1, block2 = self.nodes[2].generate(2)
60+        self.sync_all()
61+        # We should be able to get the raw transaction by providing the correct block
    promag commented at 2:16 pm on December 5, 2017:
    Nit, remaining comments are lowercase. Same below.
  143. in test/functional/rawtransactions.py:66 in 434526aba6 
    61+        # We should be able to get the raw transaction by providing the correct block
62+        gottx = self.nodes[0].getrawtransaction(tx, True, block1)
63+        assert_equal(gottx['txid'], tx)
64+        assert_equal(gottx['in_active_chain'], True)
65+        # We should not have the 'in_active_chain' flag when we don't provide a block
66+        gottx = self.nodes[0].getrawtransaction(tx, True)
    promag commented at 2:17 pm on December 5, 2017:
    Calling without blockhash in a section where the RPC is tested with a block hash :trollface: maybe move this elsewhere?
    kallewoof commented at 3:27 am on December 6, 2017:
    in_active_chain is a part of the blockhash stuff, though, so it sort of makes sense to keep the test together with the others.
  144. promag commented at 2:21 pm on December 5, 2017: member
    Mind some nits.
  145. TheBlueMatt commented at 6:29 pm on December 5, 2017: member
    utACK 434526aba680cb73208e018a02827d51a71cfff6
  146. in src/rpc/rawtransaction.cpp:146 in b167951677 outdated 
    141+            + HelpExampleCli("getrawtransaction", "\"mytxid\" true \"myblockhash\"")
142         );
143 
144     LOCK(cs_main);
145 
146+    bool in_active_chain = true;
    MarcoFalke commented at 11:44 pm on December 5, 2017:

    nit: Should not be initialized, to aid static analysers.

    Strike that. gcc would throw a warning.

  147. MarcoFalke commented at 0:24 am on December 6, 2017: member
    utACK 434526aba680cb73208e018a02827d51a71cfff6
  148. MarcoFalke commented at 2:22 am on December 6, 2017: member

    Sorry for the sloppy review. I messed up the parameter order when testing. Also gcc complains about my nit-fix.

    Tested ACK now:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3Tested ACK 434526aba680cb73208e018a02827d51a71cfff6
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIcBAEBCgAGBQJaJ1PQAAoJENLqSFDnUoslonMQAJz8bVoePuyYUgOHkbGIGemj
 74yUyywvp34A4BocYrczH9Im6q+/VBoLrxd+7BqcrhWclHC2G7tvVircgXS/O/RHE
 8SrEjox5dpLnQ5vW2Ik1hZkdlWvJvn6KBPRqLxZ8qLVFE2ym9BapDhZy33kePh4If
 9BPBalz7U/1rMz2p2a7yKLo26+MArSwt9kiGebuzgUeaUREjn3gQhaouo4mGSVN59
103Fc1zw3TS1zRuXek/EGL6uuRNQsZZkS08maHtZcsigtwS9drrekrPgx2LswvqGoN
11HNkF3AkVK2RZ96cvyG1DbGPG8PkY+EQxO97de2Jlq8/8qQ7wu/BrwJvhjGpdoqGI
12SL2pA9KgPfWDTDr13Tcavm+iGk+SbI99Hk+MgsyRArplPJYUtTi43WID/uRaydyE
13mCykCPFHkDehVIb8FL/MxSCyBzqJxn4IGXL/Q4A+BbbEYgu1Yi8xtE1o/sCqHyvD
14qoKlj1ns9JhNOIpdZNhaWvVb30VuugcZh1/I7CE4NJINdaP7oYKLS0JCiBjwxLKN
156lvBYfoJmNLOvEOiB5DGeQEtnU/mBeGrDgoRddNpnKWRQMOIKi53aKvmeJ4XNw0T
16UcIAxaj6c+lZNKYeedePcuRKapqIW9fEQ9IAI8+duRisl6TBcn7/0bJHjWLgy062
17NtvMJ3zBCmEhX3Vs4km5
18=6+iY
19-----END PGP SIGNATURE-----
  149. MarcoFalke commented at 3:30 am on December 6, 2017: member
    @jtimon @jnewbery Mind to re-ACK?
  150. jonasschnelli commented at 7:09 am on December 6, 2017: contributor
    utACK 434526aba680cb73208e018a02827d51a71cfff6
  151. laanwj merged this on Dec 6, 2017
  152. laanwj closed this on Dec 6, 2017
  153. laanwj referenced this in commit 497d0e014c on Dec 6, 2017
  154. jnewbery commented at 10:22 pm on December 6, 2017: member
    Post-merge tested ACK. I have one style nit, which I can open a tidy-up PR for if people agree.
  155. kallewoof deleted the branch on Dec 7, 2017
  156. laanwj referenced this in commit 3e50024120 on Dec 7, 2017
  157. promag commented at 1:24 am on February 22, 2018: member
    For reference, This was proposed (and closed) in #2421.
  158. kallewoof commented at 4:23 am on February 22, 2018: member
    @promag I believe the concerns raised in #2421 are not relevant anymore / in this implementation.
  159. gandrewstone referenced this in commit a41b8d26bc on Mar 20, 2019
  160. PastaPastaPasta referenced this in commit 8f743245df on Feb 13, 2020
  161. PastaPastaPasta referenced this in commit 6893b12c29 on Feb 13, 2020
  162. PastaPastaPasta referenced this in commit 85784a7cef on Feb 27, 2020
  163. PastaPastaPasta referenced this in commit 7ec4952dbd on Feb 27, 2020
  164. PastaPastaPasta referenced this in commit 8e8e06a597 on Feb 27, 2020
  165. PastaPastaPasta referenced this in commit 6cf2ea19b2 on Feb 27, 2020
  166. zkbot referenced this in commit 60331b9e83 on Nov 10, 2020
  167. ckti referenced this in commit 4ab54dbacc on Mar 28, 2021
  168. ckti referenced this in commit 1b2db678a3 on Mar 28, 2021
  169. furszy referenced this in commit ecee30d6ed on May 11, 2021
  170. DrahtBot locked this on Dec 16, 2021


kallewoof laanwj gmaxwell jonasschnelli jnewbery sipa maaku luke-jr TheBlueMatt jtimon instagibbs promag MarcoFalke

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-11-23 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me