doc, rpc: Release notes and follow-ups for #29612 #30167

1. 
   fjahr commented at 10:43 am on May 24, 2024: contributor

   This adds release notes for #29612 and addresses post-merge review comments.
2. doc: Add release notes for #29612 359967e310
3. assumeutxo: Deserialize trailing byte instead of Txid 1f1f998455
4. 
   DrahtBot commented at 10:43 am on May 24, 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, theStack

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

5. in src/test/util/chainstate.h:59 in c6c70e58cf outdated

   55@@ -56,7 +56,7 @@ CreateAndActivateUTXOSnapshot(
   56     //
   57     FILE* infile{fsbridge::fopen(snapshot_path, "rb")};
   58     AutoFile auto_infile{infile};
   59-    node::SnapshotMetadata metadata;
   60+    node::SnapshotMetadata metadata{Params().MessageStart()};
   

   ---

   ---

   

   maflcko commented at 11:11 am on May 24, 2024:

   nit: Why not use the prams from chainman? Seems more consistent, because the chainstate is also used from chainman. (Same nit on all other places)

   ---

   

   fjahr commented at 4:46 pm on May 24, 2024:

   done
6. in src/node/utxo_snapshot.h:94 in c6c70e58cf outdated

   92+            auto metadata_network{GetNetworkForMagic(message)};
   93             if (metadata_network) {
   94                 std::string network_string{ChainTypeToString(metadata_network.value())};
   95-                throw std::ios_base::failure(strprintf("The network of the snapshot (%s) does not match the network of this node (%s).", network_string, Params().GetChainTypeString()));
   96+                auto node_network{GetNetworkForMagic(m_network_magic)};
   97+                Assert(node_network);
   

   ---

   ---

   

   maflcko commented at 11:16 am on May 24, 2024:

   Not sure about turning an internal logic error in serialization into a node crash. Seems best to remove this line, because .value() will already handle the internal logic error correctly by throwing an exception.

   ---

   

   fjahr commented at 4:46 pm on May 24, 2024:

   My thinking here was that if we don’t know our own network then something must be really wrong and it’s probably a developer error. But I guess an exception is fine, so I removed the assert.

   ---

   

   maflcko commented at 4:40 pm on May 25, 2024:

   I agree that the assert should be safe here, but I think consistency-wise it makes sense to use exceptions (even for internal logic errors) over asserts/assumes in contexts where exceptions are already used and where it is safe to use them. RPC and serialization are two such areas.

   Thanks for fixing.

7. maflcko approved
8. 
   maflcko commented at 11:16 am on May 24, 2024: member

   lgtm, apart from the assert
9. assumeutxo: Add network magic ctor param to SnapshotMetadata

   This prevents SnapshotMetadata from using any globals implicitly.

   6b6084850b
10. test: Add coverage for txid coins count check when loading snapshot efc1b5be8a
11. fjahr force-pushed on May 24, 2024
12. maflcko approved
13. 
    maflcko commented at 4:47 pm on May 25, 2024: member

    utACK efc1b5be8a4696c0db19ba18316b2d4ed09e10f2
14. 
    Sjors commented at 1:02 pm on May 28, 2024: member

    I checked that efc1b5be8a4696c0db19ba18316b2d4ed09e10f2 still gives me the same snapshot as the last the time I reviewed #29612.
15. 
    fanquake commented at 11:37 am on May 30, 2024: member

    @theStack or @TheCharlatan want to have a look over the followup?
16. in doc/release-notes-29612.md:5 in 359967e310 outdated

    0@@ -0,0 +1,8 @@
    1+RPC
    2+---
    3+
    4+- The `dumptxoutset` RPC now returns the UTXO set dump in a new and
    5+  improved format. At the same time the `loadtxoutset` RPC now
    

    ---

    ---

    

    theStack commented at 2:47 pm on June 2, 2024:

    nit: not a must, but it could be interesting for users to know what exactly has improved with the new format (i.e. more space-efficient, enhanced metadata, enabling more detailed error messages etc.)

    ---

    

    fanquake commented at 9:22 am on June 3, 2024:

    I think we can make any further release note additions when the notes are consolidated / merged into the wiki.
17. theStack approved
18. 
    theStack commented at 2:48 pm on June 2, 2024: contributor

    utACK efc1b5be8a4696c0db19ba18316b2d4ed09e10f2
19. fanquake merged this on Jun 3, 2024
20. fanquake closed this on Jun 3, 2024

    ---

Contributors
fjahr DrahtBot maflcko Sjors fanquake theStack