rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block #26415

pull andrewtoth wants to merge 5 commits into bitcoin:master from andrewtoth:read-raw-block changing 9 files +65 −28
  1. andrewtoth commented at 3:27 pm on October 28, 2022: contributor

    For the getblock endpoint with verbosity=0, the rest_block REST endpoint for bin and hex, and zmq NotifyBlock we don’t have to deserialize the block since we’re just sending the raw data. This PR uses ReadRawBlockFromDisk instead of ReadBlockFromDisk to serve these requests, and only deserializes for verbosity > 0 and json REST requests. See benchmarks in #26684.

    Benchmarked using ApacheBench. Requesting block 750,000 in binary 10k times on a single core (set -rest=1 in config): ab -n 10000 -c 1 "http://127.0.0.1:8332/rest/block/0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e.bin"

    On master, mean time 15ms. On this branch, mean time 1ms.

    For RPC

    0echo '{"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e", 0]}' > /tmp/data.json
1ab -p /tmp/data.json -n 1000 -c 1 -A user:password "http://127.0.0.1:8332/"

    On master, mean time 32ms On this branch, mean time 13ms

  2. andrewtoth force-pushed on Oct 28, 2022
  3. andrewtoth marked this as a draft on Oct 28, 2022
  4. andrewtoth force-pushed on Oct 28, 2022
  5. andrewtoth force-pushed on Oct 28, 2022
  6. andrewtoth marked this as ready for review on Oct 28, 2022
  7. andrewtoth renamed this:
    rpc,rest: read raw block in getblock verbosity 0 and rest_block bin and hex
    rpc,rest: faster getblock and rest_block by reading raw block
    on Oct 28, 2022
  8. DrahtBot commented at 9:42 pm on October 28, 2022: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK achow101
    Concept ACK RandyMcMillan, kashifs, pablomartin4btc
    Stale ACK furszy, TheCharlatan

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    No conflicts as of last run.

  9. andrewtoth commented at 2:06 am on October 30, 2022: contributor

    For some more context, here is the PR that introduced ReadRawBlockFromDisk #13151.

    I see that this was attempted in #21319 as well.

  10. andrewtoth force-pushed on Oct 30, 2022
  11. andrewtoth commented at 3:41 am on October 30, 2022: contributor
    @luke-jr I wasn’t aware of #21319 until now, but I can’t comment there because it’s locked. If you want to reopen that I can review and remove the change to RPC in this PR so it will only be for REST.
  12. andrewtoth marked this as a draft on Oct 31, 2022
  13. andrewtoth force-pushed on Nov 1, 2022
  14. andrewtoth force-pushed on Nov 2, 2022
  15. andrewtoth force-pushed on Nov 3, 2022
  16. andrewtoth force-pushed on Nov 3, 2022
  17. andrewtoth force-pushed on Nov 3, 2022
  18. andrewtoth force-pushed on Nov 5, 2022
  19. DrahtBot added the label Needs rebase on Dec 8, 2022
  20. andrewtoth force-pushed on Dec 11, 2022
  21. andrewtoth marked this as ready for review on Dec 11, 2022
  22. andrewtoth renamed this:
    rpc,rest: faster getblock and rest_block by reading raw block
    rest,zmq: faster NotifyBlock and rest_block by reading raw block
    on Dec 11, 2022
  23. DrahtBot removed the label Needs rebase on Dec 11, 2022
  24. andrewtoth force-pushed on Feb 6, 2023
  25. andrewtoth renamed this:
    rest,zmq: faster NotifyBlock and rest_block by reading raw block
    rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block
    on Feb 6, 2023
  26. DrahtBot added the label Needs rebase on Apr 4, 2023
  27. achow101 requested review from TheCharlatan on Apr 25, 2023
  28. in src/rest.cpp:42 in f3c2769186 outdated 
    36@@ -37,6 +37,7 @@
37 using node::GetTransaction;
    TheCharlatan commented at 7:21 pm on April 30, 2023:
    I’m guessing the commit message should say “rest:…”?
  29. TheCharlatan commented at 7:49 pm on April 30, 2023: contributor

    Concept ACK

    Code looks good to me, but still want to run some tests.

  30. RandyMcMillan commented at 2:36 pm on May 1, 2023: contributor
    Concept ACK
  31. andrewtoth force-pushed on Jul 30, 2023
  32. andrewtoth force-pushed on Jul 30, 2023
  33. DrahtBot added the label CI failed on Jul 30, 2023
  34. DrahtBot removed the label Needs rebase on Jul 30, 2023
  35. andrewtoth commented at 3:07 am on July 30, 2023: contributor
    @TheCharlatan @RandyMcMillan rebased.
  36. DrahtBot removed the label CI failed on Jul 30, 2023
  37. in src/zmq/zmqnotificationinterface.h:8 in 6229158f77 outdated 
    4@@ -5,6 +5,7 @@
5 #ifndef BITCOIN_ZMQ_ZMQNOTIFICATIONINTERFACE_H
6 #define BITCOIN_ZMQ_ZMQNOTIFICATIONINTERFACE_H
7 
8+#include <flatfile.h>
    TheCharlatan commented at 9:16 am on August 7, 2023:
    In commit 6229158f77a7b1803f2767a089dd94dedf62b533 Nit: You could just forward-declare struct FlatFilePos without having to include flatfile (same for zmqpublishnotifier.h).
  38. TheCharlatan commented at 9:24 am on August 7, 2023: contributor
    ACK 2ccbef905fb6faa2480b49103680529dc7cfc482
  39. maflcko added the label RPC/REST/ZMQ on Aug 7, 2023
  40. DrahtBot added the label CI failed on Aug 7, 2023
  41. andrewtoth force-pushed on Aug 7, 2023
  42. andrewtoth force-pushed on Aug 7, 2023
  43. andrewtoth force-pushed on Aug 17, 2023
  44. DrahtBot removed the label CI failed on Aug 18, 2023
  45. DrahtBot added the label Needs rebase on Nov 15, 2023
  46. andrewtoth force-pushed on Nov 26, 2023
  47. DrahtBot removed the label Needs rebase on Nov 26, 2023
  48. in src/zmq/zmqnotificationinterface.h:29 in 8678db5ec4 outdated 
    25     virtual ~CZMQNotificationInterface();
26 
27     std::list<const CZMQAbstractNotifier*> GetActiveNotifiers() const;
28 
29-    static std::unique_ptr<CZMQNotificationInterface> Create(std::function<bool(CBlock&, const CBlockIndex&)> get_block_by_index);
30+    static std::unique_ptr<CZMQNotificationInterface> Create(std::function<bool(CBlock&, const CBlockIndex&)> get_block_by_index, std::function<bool(std::vector<uint8_t>&, const FlatFilePos&)> get_raw_block);
    TheCharlatan commented at 8:42 am on November 27, 2023:
    Nit: #include <vector>
  49. TheCharlatan approved
  50. TheCharlatan commented at 8:48 am on November 27, 2023: contributor
    Re-ACK a13264fb861139a38b70634d0885101ebaa24ed6
  51. kashifs commented at 11:13 pm on November 28, 2023: contributor

    Concept ACK

    I compiled from source, read over the code changes, and ran the rest and RPC benchmark tests. I didn’t get much difference between the mean times when running the benchmark tests on the 2 different branches. Does testing the different branches involve something more than checking out the different branches (master and read-raw-block) on my local computer? Thanks!

  52. andrewtoth commented at 11:38 pm on November 28, 2023: contributor

    Does testing the different branches involve something more than checking out the different branches (master and read-raw-block) on my local computer? @kashifs after checkout of each branch, you must again compile from source, and shut down the running bitcoind and then run the newly compiled binary at ./src/bitcoind.

  53. kashifs commented at 1:00 pm on November 30, 2023: contributor
    @andrewtoth, okay thanks. I did as you suggested and was able to get similar speed improvements when testing the rest command. 10.260ms on branch master and 1.014ms on branch bitcoin-read-raw-block. I only see improvements when running the rest testing command more than once though. Is this the expected behavior? I hope it’s not due to some caching or other improvements unrelated to your code updates. Also, I was not able to appreciate any improvements when running the RPC command. Could there be something else that I’m missing? Thanks again for your time.
  54. andrewtoth commented at 10:37 pm on November 30, 2023: contributor

    @kashifs make sure to wait for the log line initload thread exit to be printed in the debug.log file when restarting bitcoind before running the benchmarks.

    For RPC, does the following command return block data if you enter password when prompted for the password?

    0curl --user user --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09"]}' -H 'content-type: text/plain;' http://127.0.0.1:8332/

    If not, you do not have the correct RPC credentials.

  55. kashifs commented at 1:11 pm on December 1, 2023: contributor
    @andrewtoth, that worked! It was an issue with RPC credentials. Now all my testing leads to results similar to yours. Thanks again, for all of your help with this.
  56. pablomartin4btc commented at 9:38 pm on December 6, 2023: member

    Concept ACK

    I’d like to run the bench using #26684 which I have pending of reviewing.

  57. DrahtBot added the label Needs rebase on Dec 12, 2023
  58. achow101 referenced this in commit 00bf4a1711 on Jan 2, 2024
  59. in src/rpc/blockchain.cpp:761 in a13264fb86 outdated 
    757@@ -736,6 +758,10 @@ static RPCHelpMan getblock()
758         }
759     }
760 
761+    if (verbosity <= 0 && !RPCSerializationWithoutWitness()) {
    maflcko commented at 10:49 am on January 5, 2024:
    rpc ser-flags were removed
    pablomartin4btc commented at 2:29 pm on January 5, 2024:
    Thought was due to #28438 & #28448… but just found #28890, nice! This PR required rebase anyways.
    andrewtoth commented at 8:14 pm on January 5, 2024:
    Rebased
  60. andrewtoth force-pushed on Jan 5, 2024
  61. DrahtBot removed the label Needs rebase on Jan 5, 2024
  62. andrewtoth force-pushed on Jan 7, 2024
  63. DrahtBot added the label CI failed on Jan 7, 2024
  64. andrewtoth force-pushed on Jan 8, 2024
  65. in src/rpc/blockchain.cpp:611 in adf6766bd8 outdated 
    605+            throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)");
606+        }
607+        pos = blockindex.GetBlockPos();
608+    }
609+
610+    if (!blockman.ReadRawBlockFromDisk(data, pos)) {
    maflcko commented at 8:52 pm on January 9, 2024: 
     0 node0 stderr node/blockstorage.cpp:1070:15: runtime error: unsigned integer overflow: 0 - 8 cannot be represented in type 'unsigned int'
 1    [#0](/bitcoin-bitcoin/0/) 0x561baaf1fb61 in node::BlockManager::ReadRawBlockFromDisk(std::vector<unsigned char, std::allocator<unsigned char>>&, FlatFilePos const&) const src/node/blockstorage.cpp:1070:15
 2    [#1](/bitcoin-bitcoin/1/) 0x561bab1a45bf in GetRawBlockChecked(node::BlockManager&, CBlockIndex const&) src/rpc/blockchain.cpp:610:19
 3    [#2](/bitcoin-bitcoin/2/) 0x561bab1a45bf in getblock()::$_0::operator()(RPCHelpMan const&, JSONRPCRequest const&) const src/rpc/blockchain.cpp:760:43
 4    [#3](/bitcoin-bitcoin/3/) 0x561bab1a45bf in UniValue std::__invoke_impl<UniValue, getblock()::$_0&, RPCHelpMan const&, JSONRPCRequest const&>(std::__invoke_other, getblock()::$_0&, RPCHelpMan const&, JSONRPCRequest const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/invoke.h:61:14
 5    [#4](/bitcoin-bitcoin/4/) 0x561bab1a45bf in std::enable_if<is_invocable_r_v<UniValue, getblock()::$_0&, RPCHelpMan const&, JSONRPCRequest const&>, UniValue>::type std::__invoke_r<UniValue, getblock()::$_0&, RPCHelpMan const&, JSONRPCRequest const&>(getblock()::$_0&, RPCHelpMan const&, JSONRPCRequest const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/invoke.h:114:9
 6    [#5](/bitcoin-bitcoin/5/) 0x561bab1a45bf in std::_Function_handler<UniValue (RPCHelpMan const&, JSONRPCRequest const&), getblock()::$_0>::_M_invoke(std::_Any_data const&, RPCHelpMan const&, JSONRPCRequest const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:290:9
 7    [#6](/bitcoin-bitcoin/6/) 0x561bac05ab76 in std::function<UniValue (RPCHelpMan const&, JSONRPCRequest const&)>::operator()(RPCHelpMan const&, JSONRPCRequest const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
 8    [#7](/bitcoin-bitcoin/7/) 0x561bac05ab76 in RPCHelpMan::HandleRequest(JSONRPCRequest const&) const src/rpc/util.cpp:620:20
 9    [#8](/bitcoin-bitcoin/8/) 0x561bab1eb25a in CRPCCommand::CRPCCommand(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)::operator()(JSONRPCRequest const&, UniValue&, bool) const src/./rpc/server.h:107:91
10    [#9](/bitcoin-bitcoin/9/) 0x561bab44d73d in std::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
11    [#10](/bitcoin-bitcoin/10/) 0x561bab44d73d in ExecuteCommand(CRPCCommand const&, JSONRPCRequest const&, UniValue&, bool) src/rpc/server.cpp:539:20
12    [#11](/bitcoin-bitcoin/11/) 0x561bab44d73d in ExecuteCommands(std::vector<CRPCCommand const*, std::allocator<CRPCCommand const*>> const&, JSONRPCRequest const&, UniValue&) src/rpc/server.cpp:504:13
13    [#12](/bitcoin-bitcoin/12/) 0x561bab44a1f1 in CRPCTable::execute(JSONRPCRequest const&) const src/rpc/server.cpp:524:13
14    [#13](/bitcoin-bitcoin/13/) 0x561bab773af2 in HTTPReq_JSONRPC(std::any const&, HTTPRequest*) src/httprpc.cpp:203:40
15    [#14](/bitcoin-bitcoin/14/) 0x561bab79d53d in std::function<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&)>::operator()(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
16    [#15](/bitcoin-bitcoin/15/) 0x561bab79d53d in HTTPWorkItem::operator()() src/httpserver.cpp:60:9
17    [#16](/bitcoin-bitcoin/16/) 0x561bab7a3246 in WorkQueue<HTTPClosure>::Run() src/httpserver.cpp:115:13
18    [#17](/bitcoin-bitcoin/17/) 0x561bab78c34c in HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) src/httpserver.cpp:404:12
19    [#18](/bitcoin-bitcoin/18/) 0x7f97b6d245c2  (/lib/x86_64-linux-gnu/libstdc++.so.6+0xe65c2) (BuildId: 55cb686f6ed7c122093213afe89a2f6fd29f6347)
20    [#19](/bitcoin-bitcoin/19/) 0x561baabfef2e in asan_thread_start(void*) asan_interceptors.cpp.o
21    [#20](/bitcoin-bitcoin/20/) 0x7f97b69a5ac9  (/lib/x86_64-linux-gnu/libc.so.6+0x97ac9) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
22    [#21](/bitcoin-bitcoin/21/) 0x7f97b6a3647b  (/lib/x86_64-linux-gnu/libc.so.6+0x12847b) (BuildId: f0b834daa3d05a80967e9ec2f990a1ea71c958fa)
23SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow node/blockstorage.cpp:1070:15 in
    andrewtoth commented at 0:57 am on January 11, 2024:
    Fixed. Apparently IsBlockPruned is not sufficient to know whether we can read the block or not. Getting the file position of a block that we don’t have returns us a null FlatFilePos, and ReadBlockFromDisk fails on opening a file at null position. ReadRawBlockFromDisk instead first tries to decrement nPos by 8 before trying to open the file, which results in UB due to nPos being unsigned and 0 for a null position.
  66. andrewtoth force-pushed on Jan 11, 2024
  67. DrahtBot removed the label CI failed on Jan 11, 2024
  68. in src/node/blockstorage.cpp:1096 in 84e6a51784 outdated 
    1066@@ -1067,6 +1067,11 @@ bool BlockManager::ReadBlockFromDisk(CBlock& block, const CBlockIndex& index) co
1067 bool BlockManager::ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos) const
1068 {
1069     FlatFilePos hpos = pos;
1070+    // If nPos is less than 8 we don't have the block data and can't seek back to get meta header
1071+    // Return early to prevent undefined behavior of unsigned int underflow
1072+    if (hpos.nPos < 8) {
1073+        return error("%s: OpenBlockFile failed for %s", __func__, pos.ToString());
1074+    }
    TheCharlatan commented at 10:14 pm on January 13, 2024:
    This check seems like the right thing to do, but this could be handled by the caller, by checking if (!(pindex->nStatus & BLOCK_HAVE_DATA)), like done in net_processing.cpp:2292. Maybe still add this in a separate commit to clearly explain what is going and why being defensive here is an improvement?
    andrewtoth commented at 1:12 am on January 23, 2024:
    Done, added as a separate commit at the start of this PR. Yes, it could be handled by the caller, but it’s really handled by OpenBlockFile which checks for a null pos. It’s just that before that check it blindly decrements the nPos by 8. Now we check that and return the same error that would be returned by OpenBlockFile if pos is null.
  69. DrahtBot added the label CI failed on Jan 13, 2024
  70. andrewtoth force-pushed on Jan 23, 2024
  71. DrahtBot removed the label CI failed on Jan 23, 2024
  72. in src/rpc/blockchain.cpp:609 in 3f0f43d605 outdated 
    603+        LOCK(cs_main);
604+        if (blockman.IsBlockPruned(blockindex)) {
605+            throw JSONRPCError(RPC_MISC_ERROR, "Block not available (pruned data)");
606+        }
607+        pos = blockindex.GetBlockPos();
608+    }
    furszy commented at 4:04 pm on February 1, 2024:

    As we only care about the data existence here, what about doing a more general check:

    0if (!(blockindex.nStatus & BLOCK_HAVE_DATA)) {
1    throw JSONRPCError(RPC_MISC_ERROR, "Block not available");
2}

    (Unless you want to keep the same error message as before)

    andrewtoth commented at 10:43 pm on February 21, 2024:
    I think this check is specifically done to see if the block is pruned, hence the error message (pruned data). This is taken verbatim from the above function, GetBlockChecked, which all it does inside the locked block is check if it is pruned or not. Here we also extract the FlatFilePos. However, if blockindex.nStatus & BLOCK_HAVE_DATA is false then the call to ReadRawBlockFromDisk will fail and throw the message Block not available. The first commit addresses the undefined behavior part of calling ReadRawBlockFromDisk with a null FlatFilePos, which is safe to do now.
  73. in src/rpc/blockchain.cpp:764 in 3f0f43d605 outdated 
    763     if (verbosity <= 0) {
764-        DataStream ssBlock;
765-        ssBlock << TX_WITH_WITNESS(block);
766-        std::string strHex = HexStr(ssBlock);
767+        std::string strHex{HexStr(block_data)};
768         return strHex;
    furszy commented at 4:07 pm on February 1, 2024:

    In 3f0f43d605b44:

    could inline it:

    0if (verbosity <= 0) {
1    return HexStr(block_data);
2}
    andrewtoth commented at 10:39 pm on February 21, 2024:
    Done
  74. in src/rpc/blockchain.cpp:761 in 3f0f43d605 outdated 
    756@@ -734,15 +757,17 @@ static RPCHelpMan getblock()
757         }
758     }
759 
760-    const CBlock block{GetBlockChecked(chainman.m_blockman, *pblockindex)};
761+    const std::vector<uint8_t> block_data{GetRawBlockChecked(chainman.m_blockman, *pblockindex)};
    furszy commented at 4:12 pm on February 1, 2024:

    In 3f0f43d60:

    A topic about this change:

    GetBlockChecked() internally checks the block proof of work and the signet signature after reading the data from disk. The new GetRawBlockChecked() does not perform this checks.

    It’s not an issue for me since the block wouldn’t have been stored if the PoW was invalid, and I don’t think that the getblock RPC command is the appropriate place to check for corruption or tampered data. However, it would be nice to mention these changes in the commit description.

    andrewtoth commented at 10:43 pm on February 21, 2024:
    Done, added a commit description addressing this.
  75. in src/rest.cpp:319 in 3d7ae60f21 outdated 
    307@@ -307,32 +308,33 @@ static bool rest_block(const std::any& context,
308         if (chainman.m_blockman.IsBlockPruned(*pblockindex)) {
309             return RESTERR(req, HTTP_NOT_FOUND, hashStr + " not available (pruned data)");
310         }
311+        pos = pblockindex->GetBlockPos();
312     }
313 
314-    if (!chainman.m_blockman.ReadBlockFromDisk(block, *pblockindex)) {
315+    std::vector<uint8_t> block_data{};
316+    if (!chainman.m_blockman.ReadRawBlockFromDisk(block_data, pos)) {
    furszy commented at 4:35 pm on February 1, 2024:

    In 3d7ae60f21:

    Same discussion as with the previous commit (https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1474722573). ReadBlockFromDisk() checks PoW and the signet signature after reading the block from disk while ReadRawBlockFromDisk() doesn’t.

    I agree on not performing these checks but would be nice to describe the changes in the commit description.

    andrewtoth commented at 10:43 pm on February 21, 2024:
    Done, added a commit description addressing this.
  76. andrewtoth force-pushed on Feb 21, 2024
  77. andrewtoth commented at 11:42 pm on February 21, 2024: contributor
    @furszy @TheCharlatan updated with both your suggestions, thanks!
  78. furszy commented at 10:51 pm on March 4, 2024: member
    Code ACK 4fc91b38
  79. DrahtBot requested review from pablomartin4btc on Mar 4, 2024
  80. DrahtBot requested review from TheCharlatan on Mar 4, 2024
  81. TheCharlatan approved
  82. TheCharlatan commented at 1:28 pm on March 6, 2024: contributor
    Re-ACK 4fc91b38ae8c715e0ebf9abcb6da17044fe418b3
  83. achow101 commented at 4:20 pm on March 12, 2024: member
    ACK 4fc91b38ae8c715e0ebf9abcb6da17044fe418b3
  84. DrahtBot requested review from achow101 on Mar 12, 2024
  85. in src/node/blockstorage.cpp:1073 in 3f086a9353 outdated 
    1066@@ -1067,6 +1067,11 @@ bool BlockManager::ReadBlockFromDisk(CBlock& block, const CBlockIndex& index) co
1067 bool BlockManager::ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos) const
1068 {
1069     FlatFilePos hpos = pos;
1070+    // If nPos is less than 8 the pos is null and we don't have the block data
1071+    // Return early to prevent undefined behavior of unsigned int underflow
1072+    if (hpos.nPos < 8) {
1073+        return error("%s: OpenBlockFile failed for %s", __func__, pos.ToString());
    achow101 commented at 4:23 pm on March 12, 2024:

    In 3f086a9353191a5df03aa595bdbb42c2195a23d2 “blockstorage: check nPos in ReadRawBlockFromDisk before seeking back”

    Silent merge conflict here:

    0../../../src/node/blockstorage.cpp: In member function ‘bool node::BlockManager::ReadRawBlockFromDisk(std::vector<unsigned char>&, const FlatFilePos&) const’:
1../../../src/node/blockstorage.cpp:1094:16: error: ‘error’ was not declared in this scope; did you mean ‘perror’?
2 1094 |         return error("%s: OpenBlockFile failed for %s", __func__, pos.ToString());
3      |                ^~~~~
4      |                perror

    error() was removed in #29236

    andrewtoth commented at 4:33 pm on March 12, 2024:
    @achow101 resolved, thanks.
  86. andrewtoth force-pushed on Mar 12, 2024
  87. blockstorage: check nPos in ReadRawBlockFromDisk before seeking back 
    ReadRawBlockFromDisk assumes a non-null pos that has an nPos >= 8.
This simple check makes the function safer to call in the future,
so callers don't need to worry about causing UB if the pos is null.
    da338aada7
  88. zmq: read raw block with ReadRawBlockFromDisk 38265cc14e
  89. test: check more details on zmq raw block response 0865ab8712
  90. rpc: read raw block in getblock and deserialize for verbosity > 0 
    Note that for speed this commit also removes the proof of work and
signet signature checks before returning the block in getblock.
It is assumed if a block is stored it will be valid.
    95ce0783a6
  91. rest: read raw block in rest_block and deserialize for json 
    Note that for speed this commit also removes the proof of work and
signet signature checks before returning the block in getblock.
It is assumed if a block is stored it will be valid.
    e710cefd57
  92. andrewtoth force-pushed on Mar 12, 2024
  93. achow101 commented at 5:01 pm on March 12, 2024: member

    re-ACK e710cefd5701cd33d1e55034b3e37cea78582733

    Verified rebase diff.

  94. DrahtBot requested review from TheCharlatan on Mar 12, 2024
  95. DrahtBot requested review from furszy on Mar 12, 2024
  96. achow101 merged this on Mar 12, 2024
  97. achow101 closed this on Mar 12, 2024
  98. andrewtoth deleted the branch on Mar 12, 2024


andrewtoth DrahtBot TheCharlatan RandyMcMillan kashifs pablomartin4btc maflcko furszy achow101


furszy TheCharlatan pablomartin4btc

Labels
RPC/REST/ZMQ

github-metadata-mirror

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