[REST] Handle UTXO retrieval when ignoring the mempool #12717

pull romanz wants to merge 2 commits into bitcoin:master from romanz:master changing 4 files +54 −27
  1. romanz commented at 4:37 pm on March 18, 2018: contributor

    Current REST API always returns empty UTXO when invoked without /checkmempool/ URL part.

    After the fix:

     0$ curl -s http://localhost:8332/rest/getutxos/0e3e2357e806b6cdb1f70b54c3a3a17b6714ee1f0e68bebb44a74b1efd512098-0.json | jq
 1{
 2  "chainHeight": 514109,
 3  "chaintipHash": "0000000000000000001fe76d1445e8a6432fd2de04261dc9c5915311dc7ad6de",
 4  "bitmap": "1",
 5  "utxos": [
 6    {
 7      "height": 1,
 8      "value": 50,
 9      "scriptPubKey": {
10        "asm": "0496b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52da7589379515d4e0a604f8141781e62294721166bf621e73a82cbf2342c858ee OP_CHECKSIG",
11        "hex": "410496b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52da7589379515d4e0a604f8141781e62294721166bf621e73a82cbf2342c858eeac",
12        "reqSigs": 1,
13        "type": "pubkey",
14        "addresses": [
15          "12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX"
16        ]
17      }
18    }
19  ]
20}

    Before the fix:

    0$ curl -s http://localhost:8332/rest/getutxos/0e3e2357e806b6cdb1f70b54c3a3a17b6714ee1f0e68bebb44a74b1efd512098-0.json | jq
1{
2  "chainHeight": 514109,
3  "chaintipHash": "0000000000000000001fe76d1445e8a6432fd2de04261dc9c5915311dc7ad6de",
4  "bitmap": "0",
5  "utxos": []
6}
  2. romanz force-pushed on Mar 18, 2018
  3. romanz force-pushed on Mar 18, 2018
  4. romanz renamed this:
    Handle UTXO retrieval when ignoring the mempool
    [REST] Handle UTXO retrieval when ignoring the mempool
    on Mar 18, 2018
  5. meshcollider added the label RPC/REST/ZMQ on Mar 18, 2018
  6. ryanofsky commented at 2:26 pm on March 19, 2018: member
    utACK b1038afceadacb640ab1fa4fd9b577bb51d45e84
  7. promag commented at 10:55 pm on March 20, 2018: member

    utACK b1038af.

    Nit, could get rid of viewDummy by using viewChain by default:

    0        LOCK2(cs_main, mempool.cs);
1
2        CCoinsViewCache& viewChain = *pcoinsTip;
3        CCoinsViewMemPool viewMempool(&viewChain, mempool);
4        CCoinsViewCache view(&viewChain);
5        if (fCheckMemPool) {
6            // switch cache backend to db+mempool in case user likes to query mempool
7            view.SetBackend(viewMempool);
8        }
  8. eklitzke commented at 1:22 am on March 21, 2018: contributor

    utACK b1038afceadacb640ab1fa4fd9b577bb51d45e84

    But since we are nitting, I would probably do it like this

    0CCoinsViewCache view(fCheckMemPool ? &viewMempool : &viewChain);

    Not a big deal either way.

  9. romanz force-pushed on Mar 21, 2018
  10. romanz commented at 6:42 am on March 21, 2018: contributor
    Thanks for the review! I have removed viewDummy as suggested.
  11. romanz force-pushed on Mar 21, 2018
  12. promag commented at 2:13 pm on March 21, 2018: member

    @eklitzke that gives:

    0rest.cpp:495:44: error: incompatible operand types ('CCoinsViewMemPool *' and 'CCoinsViewCache *')

    it needs a cast:

    0CCoinsViewCache view(fCheckMemPool ? (CCoinsView*) &viewMempool : &viewChain);

    But yeah, either way.

  13. promag commented at 2:18 pm on March 21, 2018: member

    But I wonder if it pays off dealing with both cases separately:

     0if (fCheckMemPool) {
 1    LOCK2(cs_main, mempool.cs);
 2    CCoinsViewCache& viewChain = *pcoinsTip;
 3    CCoinsViewMemPool viewMempool(&viewChain, mempool);
 4    CCoinsViewCache view(&viewMempool);
 5    ...
 6} else {
 7    LOCK(cs_main);  // no need to lock mempool!
 8    CCoinsViewCache& viewChain = *pcoinsTip;
 9    CCoinsViewCache view(&viewChain);
10    ...
11}
  14. romanz commented at 5:06 pm on March 21, 2018: contributor
    @eklitzke @promag I’m fine with both suggestions, please let me know which is the preferred one :)
  15. eklitzke commented at 11:05 pm on March 21, 2018: contributor
    The way @promag just suggested is best of all. If you want to do that you’ll have to refactor the loop out so it can be called in both code paths. You can do that by adding a new static method or using a lambda.
  16. romanz force-pushed on Mar 22, 2018
  17. romanz commented at 12:10 pm on March 22, 2018: contributor
    Thanks! I have updated the code according to the suggestion above (refactoring the loop into a lambda), and added a few more tests (to make sure that we don’t return outputs, being spent by mempool transactions).
  18. romanz force-pushed on Mar 22, 2018
  19. romanz force-pushed on Mar 22, 2018
  20. in test/functional/interface_rest.py:166 in eb0aad1c0b outdated 
    164-        json_request = '/'+txid+'-'+str(n)
165+        json_request = '/'+spending
166         json_string = http_get_call(url.hostname, url.port, '/rest/getutxos'+json_request+self.FORMAT_SEPARATOR+'json')
167         json_obj = json.loads(json_string)
168-        assert_equal(len(json_obj['utxos']), 0) #there should be an outpoint because it has just added to the mempool
169+        assert_equal(len(json_obj['utxos']), 0) #there should be no outpoint because it has just added to the mempool
    jnewbery commented at 4:07 pm on March 22, 2018:

    I think this comment could be clearer. Something like # tx not found since it is in the mempool (and /mempool not included in request

    edit: in fact, I think you could remove all the inline comments and just place a comment at the top of the section explaining what’s happening (create a transaction, check that it’s found with /checkmempool, but not found without, then confirm the transaction and check that it’s found with or without /checkmempool

  21. jnewbery commented at 4:17 pm on March 22, 2018: member

    Tested ACK eb0aad1c0b90893caadd1c96d93b3f2572301cf3 with one nit in the test, which you can take or leave.

    interface_rest.py could do with some general tidy-up, so it may be best to just leave that for a future PR.

  22. romanz commented at 8:22 pm on March 22, 2018: contributor
    Thanks for the review and the suggestion! I agree, and would be happy to fix this issue (together with a general tidying-up of interface_rest.py tests) in a following PR.
  23. in src/rest.cpp:507 in eb0aad1c0b outdated 
    519+        if (fCheckMemPool) {
520+            // use db+mempool as cache backend in case user likes to query mempool
521+            LOCK2(cs_main, mempool.cs);
522+            CCoinsViewCache& viewChain = *pcoinsTip;
523+            CCoinsViewMemPool viewMempool(&viewChain, mempool);
524+            CCoinsViewCache view(&viewMempool);
    sipa commented at 8:52 pm on March 22, 2018:
    There’s no need for an intermediary cache here anymore. You can make the lambda take a CCoinsView& instead, and either pass it viewMempool (here) or *pcoinsTip (below) directly.
    romanz commented at 9:10 pm on March 22, 2018:
    You’re right, thanks for the catch! Will amend the commit, and update this PR.
  24. jnewbery commented at 8:57 pm on March 22, 2018: member

    I agree, and would be happy to fix this issue (together with a general tidying-up of interface_rest.py tests) in a following PR.

    Great! I already started tidying up the test here: https://github.com/jnewbery/bitcoin/tree/tidy_up_rest_test . I haven’t opened a PR yet. Let me know what you think (and feel free to take the branch, modify as you see fit and open a PR if you want to take this on).

  25. romanz force-pushed on Mar 22, 2018
  26. sipa commented at 9:22 pm on March 22, 2018: member
    utACK 5e2cfcba4a46a6d39f38f03dcd30543d259f181b
  27. romanz commented at 9:39 pm on March 22, 2018: contributor
    @jnewbery The branch looks great :) I will continue the development at https://github.com/romanz/bitcoin/commits/tidy_up_rest_test in the upcoming days. Much appreciated, and thanks again!
  28. in src/rest.cpp:494 in 5e2cfcba4a outdated 
    506-                outs.emplace_back(std::move(coin));
507+        auto process_utxos = [&vOutPoints, &outs, &hits](const CCoinsView& view, const CTxMemPool& mempool) {
508+            for (const COutPoint& vOutPoint : vOutPoints) {
509+                bool hit = false;
510+                Coin coin;
511+                if (view.GetCoin(vOutPoint, coin) && !mempool.isSpent(vOutPoint)) {
    promag commented at 8:46 pm on March 24, 2018:

    Alternative:

    0Coin coin;
1const bool hit = !mempool.isSpent(vOutPoint) && view.GetCoin(vOutPoint, coin);
2hits.push_back(hit);
3if (hit) outs.emplace_back(std::move(coin));

    Also, cheapest condition to fail should come first, and I think !isSpent is the cheapest in both cases.

    romanz commented at 8:56 am on March 25, 2018:
    Thanks for the suggestion! I have updated and rebased the PR.
  29. Make CTxMemPool::isSpent() const 1fdc7c41bb
  30. [REST] Handle UTXO retrieval when ignoring the mempool 
    Current REST API always returns empty UTXO when invoked without `/checkmempool/` URL part.

After the fix:
```
$ curl -s http://localhost:8332/rest/getutxos/0e3e2357e806b6cdb1f70b54c3a3a17b6714ee1f0e68bebb44a74b1efd512098-0.json | jq
{
  "chainHeight": 514109,
  "chaintipHash": "0000000000000000001fe76d1445e8a6432fd2de04261dc9c5915311dc7ad6de",
  "bitmap": "1",
  "utxos": [
    {
      "height": 1,
      "value": 50,
      "scriptPubKey": {
        "asm": "0496b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52da7589379515d4e0a604f8141781e62294721166bf621e73a82cbf2342c858ee OP_CHECKSIG",
        "hex": "410496b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52da7589379515d4e0a604f8141781e62294721166bf621e73a82cbf2342c858eeac",
        "reqSigs": 1,
        "type": "pubkey",
        "addresses": [
          "12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX"
        ]
      }
    }
  ]
}
```

Before the fix:
```
$ curl -s http://localhost:8332/rest/getutxos/0e3e2357e806b6cdb1f70b54c3a3a17b6714ee1f0e68bebb44a74b1efd512098-0.json | jq
{
  "chainHeight": 514109,
  "chaintipHash": "0000000000000000001fe76d1445e8a6432fd2de04261dc9c5915311dc7ad6de",
  "bitmap": "0",
  "utxos": []
}
```
    9cb9af8c41
  31. romanz force-pushed on Mar 25, 2018
  32. jnewbery commented at 3:42 pm on March 26, 2018: member

    Tested ACK 9cb9af8c41e4a7abd6b3e011127c4274557ca7e4.

    Only differences are addressing @sipa’s and @promag’s review comments.

    meta point - I’m not sure if this conflicted with master and needed rebase. In general it’s better to avoid rebase if possible since it makes re-reviewing easier if old and new branches are from the same base commit.

  33. romanz commented at 5:45 pm on March 26, 2018: contributor

    In general it’s better to avoid rebase if possible since it makes re-reviewing easier if old and new branches are from the same base commit.

    Will do, thanks for the tip!

  34. laanwj merged this on Mar 27, 2018
  35. laanwj closed this on Mar 27, 2018
  36. laanwj referenced this in commit ac898b689c on Mar 27, 2018
  37. laanwj referenced this in commit 69310a342f on Apr 7, 2018
  38. PastaPastaPasta referenced this in commit bd6b83865a on Sep 27, 2020
  39. PastaPastaPasta referenced this in commit 57a5064fef on Sep 27, 2020
  40. PastaPastaPasta referenced this in commit 9909c3aca7 on Oct 14, 2020
  41. UdjinM6 referenced this in commit d69f704014 on May 21, 2021
  42. UdjinM6 referenced this in commit b1223b4925 on May 21, 2021
  43. UdjinM6 referenced this in commit 41a616e0cc on May 21, 2021
  44. UdjinM6 referenced this in commit 213e509f30 on May 22, 2021
  45. kittywhiskers referenced this in commit 64bbcbd405 on May 25, 2021
  46. MarcoFalke locked this on Sep 8, 2021


romanz ryanofsky promag eklitzke jnewbery sipa

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