rpc: dumptxoutset height parameter follow-ups (29553)

fjahr commented at 0:29 am on September 4, 2024: contributor

First, this addresses two left-over comments in #29553:

When running dumptxoutset the network gets disabled in the beginning and then re-enabled at the end. The network would be re-enabled even if the user had already disabled the network themself before running dumptxoutset. The network is now not re-enabled anymore since that might not be what the user wants.
The -rpcclienttimeout=0 option is added to loadtxoutset examples in documentation

Additionally, pablomartin4btc notified me that he found his node stuck at the invalidated height after some late testing after #29553 was merged. We could not find the actual source of the issue since his logs got lost. However, it seems likely that some kind of disruption stopped the process before the node could roll forward again. We fixed this issue for network disablement with a RAII class previously and it seems logical that this can happen the same way for the rollback part so I suggest to also fix it the same way.

An example to reproduce the issue described above as I think it happened: Remove the ! in the following line in PrepareUTXOSnapshot() to simulate an issue occurring during GetUTXOStats().

0if (!maybe_stats) {

This leaves the node in the following state on master:

 0$ build/src/bitcoin-cli -rpcclienttimeout=0 -named dumptxoutset utxo-859750.dat rollback=859750
 1error code: -32603
 2error message:
 3Unable to read UTXO set
 4$ build/src/bitcoin-cli getchaintips
 5[
 6  {
 7    "height": 859762,
 8    "hash": "00000000000000000002ec7a0fcca3aeca5b35545b52eb925766670aacc704ad",
 9    "branchlen": 12,
10    "status": "headers-only"
11  },
12  {
13    "height": 859750,
14    "hash": "0000000000000000000010897b6b88a18f9478050200d8d048013c58bfd6229e",
15    "branchlen": 0,
16    "status": "active"
17  },

(Note that the first tip is headers-only and not invalid only because I started dumptxoutset before my node had fully synced to the tip. pablomartin4btc saw it as invalid.)

DrahtBot commented at 0:29 am on September 4, 2024: 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	maflcko, pablomartin4btc, achow101

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

fjahr commented at 0:29 am on September 4, 2024: contributor

cc @pablomartin4btc

in src/rpc/blockchain.cpp:2699 in e6812ffec3 outdated

2694+public:
2695+    TemporaryRollback(NodeContext& node, const CBlockIndex* index) : m_node(node), m_invalidate_index(index) {
2696+        InvalidateBlock(*m_node.chainman, m_invalidate_index->GetBlockHash());
2697+    };
2698+    ~TemporaryRollback() {
2699+        ReconsiderBlock(*m_node.chainman, m_invalidate_index->GetBlockHash());

maflcko commented at 11:02 am on September 4, 2024:

0    ChainstateManager& m_chainman;
1    const CBlockIndex& m_invalidate_index;
2public:
3    TemporaryRollback(ChainstateManager& chainman, const CBlockIndex& index) : m_chainman{chainman}, m_invalidate_index{index} {
4        InvalidateBlock(m_chainman, m_invalidate_index.GetBlockHash());
5    };
6    ~TemporaryRollback() {
7        ReconsiderBlock(m_chainman, m_invalidate_index.GetBlockHash());

nit in e6812ffec338035a1ebe949015e95ba335c94d08: Doesn’t change the behavior here, but generally it is a bit better to avoid pointers, when the value can not be nullptr. This avoids accidental undefined behavior in the future, when the nullpointer is accidentally dereferenced.

fjahr commented at 1:49 pm on September 4, 2024:

done

maflcko commented at 1:56 pm on September 4, 2024:

Forgot the CBlockIndex * -> & ? :sweat_smile:

fjahr commented at 2:04 pm on September 4, 2024:

Ah, yeah, I overlooked that. Fixed.

in src/rpc/blockchain.cpp:2813 in e6812ffec3 outdated

2807@@ -2790,10 +2808,15 @@ static RPCHelpMan dumptxoutset()
2808         // this so we don't punish peers that send us that send us data that
2809         // seems wrong in this temporary state. For example a normal new block
2810         // would be classified as a block connecting an invalid block.
2811-        disable_network = std::make_unique<NetworkDisable>(connman);
2812+        // Don't do this if the network is already disabled because this
2813+        // automatically re-enables the network activity at the end of the
2814+        // process which may not what the user wants.

maflcko commented at 11:17 am on September 4, 2024:

0        // process which may not be what the user wants.

nit in9467de52d3befd831b16b01c332793de18cb29cc : typo

Also “Don’t do this if …” can be replaced by “Skip if …”, but either is fine.

fjahr commented at 1:49 pm on September 4, 2024:

done

fjahr renamed this:
~~rpd: dumptxoutset height parameter followups (29553)~~
rpd: dumptxoutset height parameter follow-ups (29553)
on Sep 4, 2024

fjahr renamed this:
~~rpd: dumptxoutset height parameter follow-ups (29553)~~
rpc: dumptxoutset height parameter follow-ups (29553)
on Sep 4, 2024

DrahtBot added the label RPC/REST/ZMQ on Sep 4, 2024

in src/rpc/blockchain.cpp:2854 in e6812ffec3 outdated

2854-    if (invalidate_index) {
2855-        ReconsiderBlock(*node.chainman, invalidate_index->GetBlockHash());
2856-    }
2857-    if (!error.isNull()) {
2858+    } else {
2859         throw error;

maflcko commented at 11:25 am on September 4, 2024:

nit in e6812ffec338035a1ebe949015e95ba335c94d08: Wouldn’t it be easier to just throw the error on creation, instead of creating a temporary symbol and check whether it is null and then throw it? With TemporaryRollback this should be possible now.

fjahr commented at 1:49 pm on September 4, 2024:

Yeah, much simpler, thanks, done!

maflcko approved

maflcko commented at 12:10 pm on September 4, 2024: member

review ACK e6812ffec338035a1ebe949015e95ba335c94d08 💦

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK e6812ffec338035a1ebe949015e95ba335c94d08 💦
3W+/4H97jPWfOdV0n5HR2P1oOkiKMX4xhTey8kpWlh8OcWiie4vaGV7+XKN2A/MMaEg8gluzE110nCa+zLZZ+Dg==

rpc: Don't re-enable previously disabled network after dumptxoutset

Also fixes a typo in the RPC help text.

598b9bba5a

doc: Add -rpcclienttimeout=0 to loadtxoutset examples c5eaae3b89

fjahr force-pushed on Sep 4, 2024

in src/rpc/blockchain.cpp:2843 in 769dc84852 outdated

2840@@ -2823,22 +2841,14 @@ static RPCHelpMan dumptxoutset()
2841         // be activated as the new tip and we would not get to new_tip_index.
2842         if (target_index != chainstate->m_chain.Tip()) {
2843             LogInfo("Failed to roll back to requested height, reverting to tip.\n");

maflcko commented at 2:00 pm on September 4, 2024:

nit in the last commit: Shouldn’t this be a LogWarning, so that the user knows that the log is related to an RPC error that happened? Also, it could include the name of the RPC that triggered the log.

Edit: To explain, -logsourcelocations doesn’t help here, because the function name here would just be operator().

fjahr commented at 2:07 pm on September 4, 2024:

I checked the developer notes on the definition again and I think you are right that a LogWarning is a better fit. I think it’s not a “severe problem” necessarily but it does need to be addressed by the node admin if they want to run dumptxoutset successfully.

maflcko approved

maflcko commented at 2:00 pm on September 4, 2024: member

re-ACK 769dc84852e6f14876d595a7d628cc02fa074c75 🎃

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 769dc84852e6f14876d595a7d628cc02fa074c75 🎃
3NpAXsv7bFi2YYBPoRYnRhIhn+RpXg/ohmiETp1/YL5H7ZaOZFcdFFODR6KX2Ku9cOh7cXGUwsjR2ocRElLCVCQ==

fjahr force-pushed on Sep 4, 2024

rpc: Manage dumptxoutset rollback with RAII class a3108a7c56

fjahr force-pushed on Sep 4, 2024

maflcko commented at 2:07 pm on September 4, 2024: member

re-ACK a3108a7c5692d137b70b8442b4741936277e89be 🐸

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK a3108a7c5692d137b70b8442b4741936277e89be 🐸
3NK9JYrH/xtml5V8cLt4zlJuPtyWae9nucXxnYumLdL5Ac+7ZUcB01Qzi/JwdvIIDsDoiGyVgBO2UNqBhhlH1DQ==

pablomartin4btc commented at 2:44 pm on September 4, 2024: member

cr ACK a3108a7c5692d137b70b8442b4741936277e89be

Thanks for addressing the 3 follow-ups. I’ll test this soon.

Is it worth it to add a test to produce the situation where the reconsider didn’t happen and the height stayed at the rollback/ invalidated height/ stale tip (test should fail in prod and not with this PR)?

0./src/bitcoin-cli -datadir=${AU_DATADIR} -rpcclienttimeout=0 -named dumptxoutset utxo-858000.dat rollback=858000
1{
2  "coins_written": 184931392,
3  "base_hash": "00000000000000000001b44004f45de683f7f5dc792519b47f867baac2d4bf56",
4  "base_height": 858000,
5  "path": "/home/pablo/.test_utxo_840/utxo-858000.dat",
6  "txoutset_hash": "d0b6dd9367746570658e9052ff814b864643d8aeccb2ccc81d438123242db63b",
7  "nchaintx": 1064936785
8}

 0./src/bitcoin-cli -datadir=${AU_DATADIR} getchaintips
 1[
 2  {
 3    "height": 859145,
 4    "hash": "0000000000000000000109e94f6e25bd92d6424d960427e3bd10e7c65cd1996d",
 5    "branchlen": 1145,
 6    "status": "invalid"
 7  },
 8  {
 9    "height": 858000,
10    "hash": "00000000000000000001b44004f45de683f7f5dc792519b47f867baac2d4bf56",
11    "branchlen": 0,
12    "status": "active"
13  }
14]

 0./src/bitcoin-cli -datadir=${AU_DATADIR} getblockchaininfo
 1{
 2  "chain": "main",
 3  "blocks": 858000,
 4  "headers": 858000,
 5  "bestblockhash": "00000000000000000001b44004f45de683f7f5dc792519b47f867baac2d4bf56",
 6  "difficulty": 86871474313761.95,
 7  "time": 1724377947,
 8  "mediantime": 1724371333,
 9  "verificationprogress": 0.9936410953572635,
10  "initialblockdownload": true,
11  "chainwork": "00000000000000000000000000000000000000008a60c229d43fb91def135e8e",
12  "size_on_disk": 678404041445,
13  "pruned": false,
14  "warnings": [
15    "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
16  ]
17}

Just to clarify, when this happened, testing #29553, I didn’t realised the state of the node after a few other PRs reviews I made, the snapshot file was generated completely and I still have it, perhaps I shutdown the node during the reconsiderblock process…

achow101 commented at 3:28 pm on September 4, 2024: member

ACK a3108a7c5692d137b70b8442b4741936277e89be

achow101 merged this on Sep 4, 2024

achow101 closed this on Sep 4, 2024

in src/rpc/blockchain.cpp:2790 in a3108a7c56

2786@@ -2770,6 +2787,7 @@ static RPCHelpMan dumptxoutset()
2787     CConnman& connman = EnsureConnman(node);
2788     const CBlockIndex* invalidate_index{nullptr};
2789     std::unique_ptr<NetworkDisable> disable_network;
2790+    std::unique_ptr<TemporaryRollback> temporary_rollback;

ryanofsky commented at 5:09 pm on September 4, 2024:

Not important, but it would probably make sense for both disable_network and temporary_rollback variables to be std::optional instead of std::unique_ptr since they aren’t passed around it would simplify the way they are initialized.

fjahr commented at 6:16 pm on September 4, 2024: contributor

Yo dawg, I heard you like follow-ups so I made a follow-up to my follow-up ;)

#30817 addresses the std::optional comment from @ryanofsky and adds a small test as suggested by @pablomartin4btc

pablomartin4btc commented at 6:41 pm on September 4, 2024: member

post-merge tACK https://github.com/bitcoin/bitcoin/commit/a3108a7c5692d137b70b8442b4741936277e89be

(manipulated maybe_stats as suggested and other manual tests: rolling back, shutdown node during invalidate & reconsider)

rpc: dumptxoutset height parameter follow-ups (29553) #30808

Code Coverage

Reviews