Add unauthenticated HTTP REST interface #2844

pull jgarzik wants to merge 1 commits into bitcoin:master from jgarzik:rest changing 4 files +216 −1
  1. jgarzik commented at 7:35 pm on July 22, 2013: contributor

    The beginnings of a block explorer-style API for bitcoind.

    Supported API;

    1. GET /rest/tx/TX-HASH.{dat | txt | json}

    Given a transaction hash, Returns a transaction, in binary, hex-encoded binary or JSON formats.

    1. GET /rest/block/BLOCK-HASH.{dat | txt | json}

    Given a block hash, Returns a block, in binary, hex-encoded binary or JSON formats.

    Select format by appending a “.json” (JSON) or “.txt” (hex-encoded binary serialization) or “.dat” (binary serialization) suffix to the URL.

    0    GET /rest/block/BLOCK-HASH.dat
    1    GET /rest/tx/TX-HASH.txt
    2    GET /rest/tx/TX-HASH.json
    

    The HTTP request and response are both handled entirely in-memory, thus making maximum memory usage at least 3MB (1 MB max block, plus hex encoding) per request.

    This can be easily accessed via command line cURL/wget utilities.

    The general goal of the HTTP REST interface is to access unauthenticated, public blockchain information. There is no plan to add wallet interfacing/manipulation via this API.

    For full TX query capability, one must enable the transaction index via “txindex=1” command line / configuration option.

  2. luke-jr commented at 7:44 pm on July 22, 2013: member

    I don’t see the need for a non-standard “bitcoin-format” header…

    Accept: application/x-bitcoin-block, application/json Accept-Encoding: hex

  3. petertodd commented at 8:07 pm on July 22, 2013: contributor
    @luke-jr I agree, and make the default for Accept: / be to output json so curl usually works the way people expect.
  4. sipa commented at 8:54 pm on July 22, 2013: member
    I think passing the requested format as an HTTP header is awkward and hard to use. I’d say either a /rest/tx// or ?format=. This also allows us to for example at some point build a minimal HTML interface that links to it, without breaking compatibility or changing the earlier interface.
  5. maaku commented at 9:16 pm on July 22, 2013: contributor

    Yes, please don’t invent a new HTTP header: this is exactly what the Accept is for. Allowing a ?format= GET parameter alternative covers cases where specifying headers is inconvenient or impossible.

    Using Accept-Encoding for ‘hex’ struck me as a little weird, but having read the spec again it appears to be correct. I’m not sure why you’d ever use hex though when sending raw binary data over HTTP is fine, and when that is not an option the less verbose base64 is commonplace.. If this were me writing the patch, I’d support the following formats:

    Raw/binary

    0Accept: application/octet-stream
    1?format=raw
    

    Base64-encoded binary

    0Accept: application/octet-stream
    1Accept-Encoding: base64
    2?format=base64
    

    JSON

    0Accept: application/json
    1?format=json
    

    XML

    0Accept: application/xml
    1?format=xml
    

    Note that with Accept-Encoding the server notifies the client that the requested encoding was honored by including a Content-Encoding in the response.

  6. maaku commented at 9:17 pm on July 22, 2013: contributor
    Also, I would suggest using the base url /api/v1/, for hopefully obvious reasons.
  7. jgarzik commented at 0:40 am on July 23, 2013: contributor

    The Accept/Accept-Encoding feedback seems to be in line with HTTP spec, and widely requested.

    On the version number in API: it’s already there, in one sense. The API major version number will change very infrequently – perhaps once a decade, if the bitcoind JSON-RPC compatibility is any guide.

    As such, it is trivial to direct API version 2 callers to /rest2/

    Any change outside a major compatibility break may easily be handled within the /rest/ namespace.

    If people want to bikeshed and strongly prefer /rest1/ that’s fine. Just pointing out how infrequent are major version number changes, and the fact that the current scheme already handles major version changes.

  8. jgarzik commented at 1:48 am on July 23, 2013: contributor
    Added: In the past, /api/v1 has been suggested for the RPC interface. At this point, “api” is too generic I think.
  9. jgarzik commented at 2:26 pm on July 23, 2013: contributor

    @sipa makes a fair point about requiring a header being a bit more difficult. Software such as dumb browsers do not permit easy HTTP header modification. However, query strings are bloody ugly.

    github.com-style clean URLs seem like a smart way to go, e.g.

    0    GET /rest/tx/TX-HASH/json
    

    to get the non-default JSON output.

    Easy enough to add modifiers after the TX-HASH.

  10. jgarzik commented at 2:55 pm on July 23, 2013: contributor
    Updated commits and pull req description to indicate use of “clean” URLs. Non-standard header “Bitcoin-Format” removed.
  11. maaku commented at 5:46 pm on July 23, 2013: contributor

    GET /rest/tx/TX-HASH/json

    Is that actually how github does it? TX-HASH.json might be a better choice, I think. Dumb clients which ignore the headers will assume it’s json from the extension.

  12. keo commented at 4:00 pm on August 7, 2013: none
    @jgarzik thank you! This is the beginning of a usable, clean API which appeals to merchants and PSPs. @maaku agree on providing .json instead of /json - seems to be the way everyone does it (without saying anything about whether this is good or bad).
  13. runeksvendsen commented at 4:08 pm on August 7, 2013: contributor

    Excuse me if this is out of place, but why implement this in bitcoind?

    Writing an external program that wraps bitcoind RPC calls and allows HTTP querying makes much more sense to me. As far as I can see this adds no new information retrievable from bitcoind, it only changes the protocol/format. A simple Python script should be able to do this, no?

  14. runeksvendsen commented at 8:31 pm on August 10, 2013: contributor

    Just for fun, I created a simple Python script that does this: https://github.com/runeksvendsen/btchttp/blob/master/btchttp.py

    It only supports JSON right now, but should easily extensible.

  15. gavinandresen commented at 9:36 pm on August 10, 2013: contributor
    +1 for @runeksvendsen : I’d rather ship a version of his btchttp.py in contrib/ than make core bitcoind bigger.
  16. rebroad commented at 11:49 pm on August 10, 2013: contributor

    The risk in moving this to contrib is that the majority of peers wouldn’t use it, and then when ISPs start blocking the original bitcoin protocol the network is more likely to die. What we really need is HTTPS so that it’s harder to block.

    On Sunday, August 11, 2013, Gavin Andresen wrote:

    +1 for @runeksvendsen https://github.com/runeksvendsen : I’d rather ship a version of his btchttp.py in contrib/ than make core bitcoind bigger.

    — Reply to this email directly or view it on GitHubhttps://github.com/bitcoin/bitcoin/pull/2844#issuecomment-22447903 .

  17. sipa commented at 8:27 am on August 11, 2013: member

    @rebroad This is not an interface intended to be exposed to the internet. You can do so of course, but it’s not a replacement for the P2P system (it’s more an addition to RPC). It’s just an interface to ease debugging, or help other local applications that need access to raw block/transaction data.

    Regarding whether this belongs in bitcoind, I’m in the middle. I understand the concern about not bloating bitcoind even further, but if such a feature means more people running a local bitcoind instead of relying on some centralized webservice, I’m all for it. The same goes for an address index, and a potential minimal built-in block explorer. However, I’m not sure what this adds that isn’t available already. Humans will not use this interface, IMHO, and external applications can already use getblock/getrawtransaction.

  18. jgarzik commented at 3:59 pm on August 11, 2013: contributor

    I fear several commenters here read the pull request title, and did not examine what the code actually does.

    Some salient points:

    • Dramatically easier interface for developers and general queries
    • Out of the box SSL support, already built into bitcoind
    • This pull adds functionality not available via RPC (there is no getrawblock) – thus the btchttp.py example does not provide what this pull request provides.
    • Wider selection of command line tools work out of the box with HTTP REST, versus JSON-RPC
    • Nobody will write their own proxy to accomplish this
    • A contributed proxy, shipped with bitcoind, might be used – but it is obviously a fragile, two-process unsupported solution
  19. sipa commented at 4:25 pm on August 11, 2013: member
    @jgarzik It’s clearly a compromise between usability and bloat, which may or may not be worth it (see my other comment regarding that), however, there IS a getrawblock, it’s called getblock [hash] false.
  20. jgarzik commented at 11:31 pm on August 24, 2013: contributor
    Rebased. The first commit is a cleanup candidate for immediate inclusion, too.
  21. jgarzik commented at 2:02 am on August 25, 2013: contributor

    Updated for @maaku ’s suggestion of HASH.EXTENSION, where the extension (.json, .txt, .dat) selects the format.

    Updated OP examples.

  22. sipa commented at 4:11 pm on August 25, 2013: member
    ACK
  23. in src/rest.cpp: in bc93b5ad6d outdated
    76+    if (mapBlockIndex.count(hash) == 0)
    77+        throw RESTERR(HTTP_NOT_FOUND, hashStr + " not found");
    78+
    79+    CBlock block;
    80+    CBlockIndex* pblockindex = mapBlockIndex[hash];
    81+    ReadBlockFromDisk(block, pblockindex);
    


    sipa commented at 4:13 pm on August 25, 2013:
    For compatibility with future headers-first mode, ReadBlockFromDisk may fail.
  24. jgarzik commented at 2:29 am on August 26, 2013: contributor
    Rebased for CreateNewBlock() update.
  25. jgarzik commented at 2:57 am on August 26, 2013: contributor

    JFYI: Since it was easy, I implemented the following on a side branch:

    0     GET /rest/block/template.(dat|txt)
    

    to download the binary (/hex) encoding of a miner block template. No fee or sigop information is provided, just straight CBlock and nothing else.

    It seems nice and efficient for a pool server to simply request the binary block from a trusted node, and vary bits of the header and coinbase from there.

    As noted at top, this is on a side branch, and will not be added to this pull req.

  26. jgarzik commented at 2:58 am on August 27, 2013: contributor
    Merge-ready
  27. jgarzik commented at 3:39 pm on October 2, 2013: contributor

    Rebased. Merge-ready.

    Note: I also have a HTTP REST interface for “getblocktemplate” on a local side branch.

  28. gavinandresen commented at 5:39 am on October 15, 2013: contributor

    Pull-tester error: rest.h is mentioned in the Makefile.am, but there ain’t no rest.h committed.

    Also, when testing: Is it supposed to be …TXHASH.json or TXHSAH/json ? The former complains “invalid hash”, the latter works.

  29. laanwj commented at 7:58 am on October 15, 2013: member

    I like the interface.

    Regarding bloat/redundancy, is this going to deprecate the JSON API calls for doing non-authenticated, non-wallet queries eventually?

  30. laanwj commented at 7:35 am on October 21, 2013: member
    It also makes sense to have a different interface for block chain data if we want to split the wallet off into another executable eventually. The “block chain daemon” part wouldn’t need a JSON RPC interface at all.
  31. luke-jr commented at 5:03 pm on February 21, 2014: member
    @jgarzik Needs rebase
  32. jgarzik commented at 3:59 pm on June 4, 2014: contributor
    Rebased.
  33. jtimon commented at 4:40 pm on June 4, 2014: contributor

    I agree with @laanwj that this will probably makes separating the wallet code easier. It would also make implementing full-node wallets based on a trusted bitcoind much simpler.

    It comes to mind that maybe a /rest/address/ call could be interesting as well. Maybe accepting parameters to filter the outputs. For example, only show unspent outputs for a given address or only show outputs from block X to Y. Is there any reason I’m missing (apart from the code bloat arguments) why this wouldn’t make sense?

  34. jgarzik commented at 6:08 pm on June 4, 2014: contributor
    Updated for year-old @sipa comment to check ReadBlockFromDisk() return value.
  35. jgarzik commented at 6:11 pm on June 4, 2014: contributor
    @jtimon Sure. REST is just a lens through which any query may be poured. :) These two examples, block and tx , are easy and map very well to the REST model, to the point of being compatible with wget(1), curl(1), and similar command line tools.
  36. sipa commented at 8:06 pm on June 9, 2014: member
    @jtimon There is no functionality for querying anything (unauthenticated) by address. How do you expect rest/address would work?
  37. maaku commented at 8:33 pm on June 9, 2014: contributor
    By applying the address index patch, I presume.
  38. jtimon commented at 10:37 pm on June 9, 2014: contributor
    My idea was to look at what happens when you import a private key into the wallet first and hopefully learn something there. Then basically return some json data similar to what explorers usually return in their myexplorer.con/address/ pages, a list of outputs or an empty list. Where’s the address index patch? I assumed there was an index already, or is the chain database queried by public key or private key when a key is imported? Mhumm, I’m starting to read and I fear a rescan is needed…so, yes, I guess I need that address index patch.
  39. sipa commented at 10:45 pm on June 9, 2014: member

    I started writing that address index patch, and closed the pull request because I believe it’s the wrong approach. It would be supporting building applications that rely on having all blockchain data always present (making pruning hard) and having an expensive to maintain index on top of it.

    For almost everything, that’s just the easiest solution - just querying a database - while watch-only wallets are much more scalable. Don’t use a key you’ve used before (so no need for rescanning), and just maintain transactions that interest you while watching new blocks. That’s the model I want to encourage.

    Of course, fully indexed databases are useful (especially debugging), and for some use cases they’re unfortunately inevitable. I still don’t think bitcoind is the right place for those. It should focus on becoming just the core router of the network, with as little extra functionality or responsabilities as possible. If we really need indexing, let’s put it in a tool optimized for just that.

  40. leofidus commented at 5:50 am on June 10, 2014: none
    @jtimon look at #3652. Current behaviour is to do a rescan at address import (which takes about 15 minutes).
  41. BitcoinPullTester commented at 4:08 pm on July 18, 2014: none
    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p2844_7da6e17a134e9f181bbb71aba27cc6923cff6387/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
  42. jgarzik commented at 6:32 pm on July 18, 2014: contributor
    All feedback addressed.
  43. jgarzik added the label Improvement on Jul 31, 2014
  44. jgarzik added the label Feature on Jul 31, 2014
  45. jgarzik commented at 3:22 pm on October 13, 2014: contributor
    Let’s get a go/no-go for launch.
  46. jtimon commented at 4:41 pm on October 13, 2014: contributor
    I would say go.
  47. TheBlueMatt commented at 4:51 pm on October 13, 2014: member
    I would say no, using some arbitrary concern over more crap in the process space. Once we can split off RPC using some IPC mechanism, then we can add all the new interfaces we want…Still the limited new code in this is nice, so I wont complain at all if others disagree.
  48. jtimon commented at 4:55 pm on October 13, 2014: contributor
    Besides backwards compatibility, is there any advantage for RPC over regular http rest?
  49. jgarzik commented at 5:09 pm on October 13, 2014: contributor

    @jtimon Well, avoiding your question for a moment, the idea of this interface was to design something that may safely provide anything considered “public data”, and thus run fully without authentication.

    Does it make sense to redesign RPC interface to run over HTTP REST? At this point, I would not see any value to dropping JSON-RPC or delivering the same authenticated functionality via HTTP REST, given the number of clients using it.

    Does it make sense to export other public data via HTTP REST? Sure. I was thinking get-UTXO call would be applicable to some existing, in the field use cases.

  50. jtimon commented at 5:16 pm on October 13, 2014: contributor
    Thanks, but my question was more “if we had had http rest from the beginning instead of the rpc we have, would there be any disadvantage?”
  51. jgarzik commented at 5:35 pm on October 13, 2014: contributor

    @jtimon It wouldn’t be JSON-RPC, but some less defined standard into which you must then define a set of rich-data export mechanisms (JSON!), because HTTP REST doesn’t provide that natively.

    As JSON and JSON-RPC are pretty well entrenched at this point, it is a bit of a theoretical question :)

  52. jtimon commented at 6:51 pm on October 13, 2014: contributor
    ...it is a bit of a theoretical question Yes it is, but thanks for answering it.
  53. laanwj commented at 8:12 am on October 14, 2014: member

    Does it make sense to redesign RPC interface to run over HTTP REST? At this point, I would not see any value to dropping JSON-RPC or delivering the same authenticated functionality via HTTP REST, given the number of clients using it.

    Agreed. This is for exporting stateless public data only. Not for control. not for authenticated private data.

    I’d say go.

    Once we can split off RPC using some IPC mechanism

    Splitting off an RPC mechanism using a IPC mechanism would be a bit strange as it’s essentially the same thing.

    I do agree your arbitrary concern about “crap in the process space” though, and would encourage modularity at least… but @jgarzik did a good job there. The impact to existing code is the absolute minimum.

    The only way I could see this being even more modular is if rpcserver had a signal that modules could subscribe to to handle part of the path space.

  54. in src/rpcserver.cpp: in 7da6e17a13 outdated
    906+            if (!HTTPReq_REST(conn, strURI, mapHeaders, fRun))
    907+                break;
    908+
    909         } else {
    910-            conn->stream() << HTTPError(HTTP_NOT_FOUND, false) << std::flush;
    911+            conn->stream() << HTTPReply(HTTP_NOT_FOUND, "", false) << std::flush;
    


    laanwj commented at 10:23 am on October 22, 2014:
    Why this change from HTTPError to HTTPReply?

    jgarzik commented at 4:05 am on October 24, 2014:
    @laanwj Looks like an error to me.

    sipa commented at 3:20 pm on November 18, 2014:
    Did you fix it?
  55. laanwj commented at 8:22 am on October 28, 2014: member
    Needs rebase
  56. SergioDemianLerner commented at 3:56 am on October 30, 2014: contributor
    I think you should be aware that some web browsers allow javascript code to connect to a localhost port, such as the bitcoind HTTP REST service, so it will be possible from any malicious web page in the Internet to detect the presence of bitcoind and query whatever data it provides.
  57. SergioDemianLerner commented at 4:01 am on October 30, 2014: contributor
    Since the same interface provides access to HTTPReq_JSONRPC, then a malicious web page could perform a dictionary attack on the RPC password.
  58. Add unauthenticated HTTP REST interface to public blockchain data. e2655e0ab1
  59. jgarzik force-pushed on Nov 11, 2014
  60. jgarzik commented at 9:55 am on November 11, 2014: contributor
    Rebased & fixed #include-related build breakage.
  61. jgarzik commented at 9:56 am on November 11, 2014: contributor
    @SergioDemianLerner That risk exists regardless of this interface
  62. laanwj commented at 10:37 am on November 11, 2014: member

    then a malicious web page could perform a dictionary attack on the RPC password.

    That’s going to be a veeery slow attack. A RPC thread is held up for 250ms after each wrong password try, and the number of RPC threads is finite and low.

    Anyhow, this risk has always existed. This is one of the reasons that RPC is disabled by default in Bitcoin-Qt - reduce default attack surface.

  63. jgarzik commented at 10:58 am on November 11, 2014: contributor

    Regardless, if people really want it that way, I can add the HTTP interface on a separate port, with all that entails.

    I do agree it is more likely that people might want to expose this interface to a wider selection of programs/people than RPC.

    It’s the same problem class as running multiple virtual hosts on the same httpd.

  64. laanwj commented at 11:12 am on November 11, 2014: member

    That would mean another per-instance-port to configure, another set of bind directives, another set of allowip rules… not sure I’m so happy about that.

    What I like about this pull is how little impact it has on existing code.

    But that would veer me in the direction of “implement this as an additional application/script on top of RPC” instead of inside Bitcoin Core itself (as noted by @runeksvendsen).

    Edit BTW: Encouraging people to open up the current web server code to a wider scope does have its risks, even if not on a port with RPC. For example the code can’t cope with DoS very well, but it may also have other issues which haven’t been problematic before because RPC needs to be kept private. It means this code needs additional hardening.

  65. petertodd commented at 9:20 am on November 12, 2014: contributor
    I can’t help but think how this could probably be done in just a dozen or two lines of python-bitcoinlib code… and it could be in a totally separate process, even a separate user.
  66. laanwj commented at 4:49 pm on November 12, 2014: member
    @petertodd Wouldn’t be difficult to port https://github.com/runeksvendsen/btchttp/blob/master/btchttp.py from bitcoinrpc to python-bitcoinlib.
  67. in src/rest.cpp: in e2655e0ab1
    54+            return rf_names[i].rf;
    55+
    56+    return rf_names[0].rf;
    57+}
    58+
    59+static bool ParseHashStr(string& strReq, uint256& v)
    


    sipa commented at 3:17 pm on November 18, 2014:
    const string& strReq?
  68. jgarzik merged this on Nov 18, 2014
  69. jgarzik closed this on Nov 18, 2014

  70. jgarzik referenced this in commit 9445b876bd on Nov 18, 2014
  71. TheBlueMatt commented at 9:09 pm on November 18, 2014: member
    Ummmm….WHAT? I dont see a single ACK on this pull.
  72. sipa commented at 9:11 pm on November 18, 2014: member
    There is one actually, by me, from a long time ago. Though I was surprised too that it just got merged (and several bugs got fixed just after merging even…).
  73. TheBlueMatt commented at 9:11 pm on November 18, 2014: member

    There was one irc discussion

    0<jgarzik_> wumpus, HTTP REST, yea or nay?
    1<jgarzik_> I think it's usefully small and compact, and will be used extensively by upper layer services
    2<wumpus> ACK
    

    which doesnt even read as “go ahead and merge” to me.

  74. jgarzik commented at 4:01 am on November 19, 2014: contributor

    @TheBlueMatt There have been many small snippets of discussion in email or irc across many months. That was a final poke, not the entire discussion.

    There was no objection to this being on the 0.10 list that circulated, and the general response from committers was either apathy or ack.

  75. jgarzik deleted the branch on Nov 19, 2014
  76. TheBlueMatt commented at 6:56 am on November 19, 2014: member

    @jgarzik My criticism isnt that there wasnt consensus that this should be merged, but that it was not obvious here. From reading only this commit and without spending a bunch of time searching for discussions, it looks like this was merged without review. If @wumpus or @gmaxwell could posthumously ack this, that’d be fine.

    On 11/19/14 04:01, Jeff Garzik wrote:

    @TheBlueMatt https://github.com/TheBlueMatt There have been many small snippets of discussion in email or irc across many months. That was a final poke, not the entire discussion.

    There was no objection to this being on the 0.10 list that circulated, and the general response from committers was either apathy or ack.

    — Reply to this email directly or view it on GitHub #2844 (comment).

  77. wumpus commented at 7:13 am on November 19, 2014: none
    Wrong @wumpus. That other guy really ought to not use my name on IRC, either, given my history with IRC.
  78. TheBlueMatt commented at 7:43 am on November 19, 2014: member
    ehh, sorry, meant @laanwj
  79. laanwj commented at 9:01 am on November 19, 2014: member
    @TheBlueMatt No objection - I’m still kind of ambivalent whether this belongs in core, but I like the idea of a HTTP REST standard for getting read-only public data from Bitcoin nodes. So posthumous ACK. @wumpus Let’s very much not get into a I’m older-than-you-and-have-been-using-this-name-longer turf war here. I don’t care. I think it’s possible to block mentions from a project, that would be a practical solution.
  80. sipa commented at 1:03 pm on November 20, 2014: member

    So it seems the binary data option doesn’t work (Content-Length: 0 is sent), did anyone ever test that?

    Also, it seems the interface is /rest/TYPE/HASH/FORMAT, rather than the /rest/TYPE/HASH.EXT that the discussion above suggests.

    It seems like nobody actually tried this once (I’m at fault too for that, and perhaps my very old review was too fast…). I started reviewing it again when the discussion on IRC about it started a few days ago, but then it was suddenly merged.

  81. jonasschnelli commented at 1:06 pm on November 20, 2014: contributor
    I started testing some hours ago and start now address issues at #5326.
  82. jgarzik commented at 2:17 pm on November 20, 2014: contributor

    @sipa Yes, it was extensively tested pre-rebase.

    Very little testing post-rebase, and HTTPReply() was an area that had a prior rebase bug as well.

  83. petertodd commented at 7:17 pm on November 20, 2014: contributor
    For the record in addition to other objections others have already covered, it bothers me that we merged a whole new set of features without a set of unittests for it. In particular, testing this feature with a simple Python script that just grabbed some sample URL’s and checked the results against vectors would have been really easy and would have caught the rebase breakage.
  84. jtimon commented at 3:00 pm on November 21, 2014: contributor
    @petertodd maybe something to add to #5326.
  85. jonasschnelli commented at 3:05 pm on November 21, 2014: contributor
    I’m already writing some unit tests for the REST interface. But i wait with the pull until #5326 gets merged. I don’t want to push all the times and “clear” previous ACKs.
  86. petertodd commented at 6:25 pm on November 21, 2014: contributor

    @jonasschnelli Thanks! Yeah, that’s the kind of thing that should have been a part of the original pull-req.

    For instance, I would consider the lack of unit tests for my own pull-req #2340 to be a reasonable reason to NACK merge it, even though it’s much simplier and creating unit tests for it is much more difficult. To skip that step in a case where writing tests is pretty easy is something I think we should avoid doing in the future.

  87. IntegralTeam referenced this in commit b5bc7c9dac on Jun 4, 2019
  88. DrahtBot locked this on Sep 8, 2021

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-17 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me