In commit "interfaces: expose UTXO snapshot loading in node interface" (de11e3b89f8aba9a90a3419086807e5fee0f941d)

Note with #10102 if the GUI & node are running in separate processes, it could be a awkward for the gui process to create an AutoFile object and pass it to the node like this. Technically it could probably work: the AutoFile could be passed as a file descriptor or a (path, position) tuple. But it doesn't seem like a clean separation of responsibilities if the GUI process is reading the beginning of the snapshot file and the node process is reading the rest of the file.

Probably current interface is ok, but it might be more ideal if node provided a more generic loadSnapshot method and the GUI didn't directly access the file:

class Node
{
    // ...
    std::unique_ptr<Snapshot> loadSnapshot(const fs::path& path) = 0;
    // ...
};

class Snapshot
{
public:
    virtual ~Snapshot() = default;
    node::SnapshotMetadata getMetadata() = 0;
    bool activate() = 0;
};

Thanks for the review... will update soon addressing this

with e79c50c updated the node.h and added snapshot.h to the /interfaces.
Also updated interfaces.cpp with a SnapshotImpl class to process the path and extracts the metada and activate the snapshot.
@ryanofsky let me know if that's what you had in mind?

re: #33117 (comment)

Thanks! I think b464f388197c5c31937d309d817d38c4b4849a91 is an improvement over previous de11e3b89f8aba9a90a3419086807e5fee0f941d because it passes an fs::path argument from the GUI to the node instead of an AutoFile file argument. This is nice because it lets node be fully responsible for parsing the snapshot file instead of relying on the GUI.

One comment on b464f388197c5c31937d309d817d38c4b4849a91 is that it looks like the Snapshot class is not actually necessary in its current form. The reason I suggested adding a Snapshot class above was that I thought GUI wanted to display SnapshotMetadata information before activating the snapshot, and a Snapshot class could provide separate getMetadata() and activate() methods to allow that. But since the Snapshot class only has a single activate() method in b464f388197c5c31937d309d817d38c4b4849a91, it looks like the class is unnecessary, and you could just have a plain Node::activateSnapshot(fs::path) method that returns bool instead of a Snapshot.

Any approach is fine though, so you should choose whatever seems to make the most sense.

Thanks for taking a look!
RE:

The reason I suggested adding a Snapshot class above was that I thought GUI wanted to display SnapshotMetadata information before activating the snapshot, and a Snapshot class could provide separate getMetadata()

I took some time to test various scenarios downstream in the gui-qml project. While we aren't directly utilizing the snapshot data at the moment, I believe your initial instincts about introducing a Snapshot class were well-founded. This abstraction will become increasingly valuable as we begin to expose the dump/generate snapshot functionalities trough the GUI.

In 3c8b578836dd0d8438a8aac81a1166df84811562 interfaces: expose UTXO snapshot loading in node interface: a good way to add test coverage to this new interface would be to use it in the loadtxoutset RPC.

For an example, see the various uses of EnsureMining(node);

Thanks @Sjors for providing feedback and suggesting to add test coverage via the RPC...

Looking into it now and will update shortly

a good way to add test coverage to this

refactored loadtxoutset to utilize the new snapshot interface...