multiprocess: Add capnp wrapper for Chain interface #29409

pull ryanofsky wants to merge 4 commits into bitcoin:master from ryanofsky:pr/ipc-chain changing 12 files +678 −6
  1. ryanofsky commented at 1:54 pm on February 8, 2024: contributor

    Changes making it possible to call interface::Chain over a socket.

    Note: The changes in this PR allow a bitcoin-node process to be controlled over a socket. The socket can either be an UNIX socket created by specifying the -ipcbind option, or it can be a socketpair created by a controlling process and passed to bitcoin-node via the -ipcfd=<n> with a file descriptor number. This PR is a dependency of #10102 which allows the wallet code to run in a separate process and use the Chain interface.

    This PR is part of the process separation project.

  2. DrahtBot commented at 1:54 pm on February 8, 2024: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29409.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK TheCharlatan
    Concept ACK josibake, darosior
    Ignored review cbergqvist

    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.

  3. ryanofsky marked this as ready for review on Feb 8, 2024
  4. DrahtBot added the label CI failed on Feb 8, 2024
  5. DrahtBot commented at 3:41 pm on February 8, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/21365328047

  6. ryanofsky force-pushed on Feb 8, 2024
  7. DrahtBot removed the label CI failed on Feb 8, 2024
  8. in src/ipc/capnp/common-types.h:199 in eeb441ae7c outdated 
    219+template <typename LocalType, typename Value, typename Output>
220+void CustomBuildField(TypeList<util::Result<LocalType>>, Priority<1>, InvokeContext& invoke_context, Value&& value,
221+                      Output&& output)
222+{
223+    auto result = output.init();
224+    if (value) {
    cbergqvist commented at 10:12 pm on February 26, 2024:
    Nit: compiles with value.has_value() here, which is clearer than operator bool(). But maybe you were intending to support types which don’t have .has_value()?
    ryanofsky commented at 3:25 pm on February 28, 2024:

    re: #29409 (review)

    Nit: compiles with value.has_value() here, which is clearer than operator bool(). But maybe you were intending to support types which don’t have .has_value()?

    I mostly just don’t agree it is clearer and I like the “if value, use value” pattern from c++ and python. I think it removes noise and makes code easier to read, also makes it easier for code to evolve, for example updating code using a plain pointer to switch to smart pointer.

    In this case I am looking forward to this code working with ResultPtr from #26022, where operator bool and has_value have different meanings. Operator bool in that case is true if a pointer is contained AND the pointer value is not null. has_value is true in that case if a pointer is contained, whether or not it is null. After #26022, this code would look something like:

    0if (value) {
1     BuildField(..., ValueAccessor, *value);
2}
3if (!value.has_value()) {
4    BuildField(..., FailureAccessor, value.GetFailure());
5}
6BuildField(..., ErrorsAccessor, value.GetErrors());
7BuildField(..., WarningsAccessor, value.GetWarnings());

    And if value is a ResultPtr the first line would need to be if (value) not if value.has_value() to work.

  9. in src/ipc/capnp/chain-types.h:15 in 2fd10424c0 outdated 
    10+#include <ipc/capnp/handler.capnp.proxy-types.h>
11+#include <interfaces/chain.h>
12+#include <policy/fees.h>
13+#include <rpc/server.h>
14+
15+#include <any>
    cbergqvist commented at 8:33 am on February 27, 2024:
    Builds fine without #include <any> using g++ on Linux?
    ryanofsky commented at 3:26 pm on February 28, 2024:

    re: #29409 (review)

    Builds fine without #include <any> using g++ on Linux?

    Good catch, I think this must have been left over from a previous version of this file. Removed now.

  10. in src/ipc/capnp/common-types.h:338 in 2fd10424c0 outdated 
    333+}
334+
335+//! Generic ::capnp::Data field builder for any class that a Span can be
336+//! constructed from, particularly BaseHash and base_blob classes and
337+//! subclasses. It's also used to serialize vector<unsigned char>
338+//! GCSFilter::ElementSet set elements.
    cbergqvist commented at 1:51 pm on February 28, 2024:
    Should this say …“vector<unsigned char> and GCSFilter::ElementSet set elements.”?
    ryanofsky commented at 3:30 pm on February 28, 2024:

    re: #29409 (review)

    Should this say …“vector and GCSFilter::ElementSet set elements.”?

    I think it’s technically correct because the the vectors are the set elements but I changed the comment to make it clearer.

  11. cbergqvist commented at 1:56 pm on February 28, 2024: contributor
    I’m eager for multiprocess to get in, but unable to do in-depth reviews in tolerable time due to lack of domain-knowledge. Did a surface level review of 2fd1042. Hope you don’t mind.
  12. ryanofsky force-pushed on Feb 28, 2024
  13. ryanofsky commented at 4:14 pm on February 28, 2024: contributor

    Thanks for the review!

    Updated 2fd10424c04397e50c6683a82db8a0bb68e0f6cc -> 41831abe06d388ed18b5dbed739bc8fe76457f86 (pr/ipc-chain.2 -> pr/ipc-chain.3, compare) with suggested changes.

  14. cbergqvist approved
  15. cbergqvist commented at 4:58 pm on February 28, 2024: contributor
    Thanks for incorporating some of my suggestions. Surface-level ACK of 41831ab. :)
  16. DrahtBot added the label CI failed on Feb 28, 2024
  17. in src/ipc/capnp/common-types.h:50 in eeb441ae7c outdated 
    37+//! Construct a ParamStream wrapping data stream with serialization parameters
38+//! needed to pass transaction and address objects between bitcoin processes.
39+template<typename S>
40+auto Wrap(S& s)
41+{
42+    return ParamsStream{s, TX_WITH_WITNESS, CAddress::V2_NETWORK};
    ariard commented at 2:54 am on March 7, 2024:
    i think Wrap could be parameterized with any CAddress version. e.g useful if someone wanna test the native crypto noise framework between bitcoin processes. or in the future experiment with better crypto-primitives for bip324.
    ryanofsky commented at 4:29 pm on July 26, 2024:

    re: #29409 (review)

    i think Wrap could be parameterized with any CAddress version. e.g useful if someone wanna test the native crypto noise framework between bitcoin processes. or in the future experiment with better crypto-primitives for bip324.

    For now the Wrap function is basically a workaround for not having a serialization option that says “serialize all the data in this object to a stream, I don’t care about the specific serialization format.” So the formats here just chosen to preserve all data, and don’t really matter otherwise.

    But I could imagine use-cases like you are suggesting where we may want to customize which serialization format is used depending on the IPC connection. I think it would not be that hard to do by adding parameters to Wrap, which is not called many places, or by writing different CustomReadField/BuildField overloads which do not call Wrap. For now though the easiest thing is for the wrapped stream to just hardcode parameters that serialize all the data.

  18. ariard commented at 2:57 am on March 7, 2024: contributor
    being able to work with any socket (TCP or UNIX) makes a lot of sense. you could have bitcoin-node on some secure computing host. and then one or more bitcoin-walet on over-the-network hosts.
  19. DrahtBot marked this as a draft on Apr 17, 2024
  20. DrahtBot added the label Needs rebase on May 15, 2024
  21. ryanofsky force-pushed on Jun 11, 2024
  22. ryanofsky marked this as ready for review on Jun 11, 2024
  23. DrahtBot removed the label Needs rebase on Jun 11, 2024
  24. ryanofsky force-pushed on Jun 11, 2024
  25. DrahtBot removed the label CI failed on Jun 12, 2024
  26. TheCharlatan commented at 10:14 am on July 5, 2024: contributor
    I’m not sure what a good approach for reviewing this might be. How can I validate the changes without tests? Should I be running this as part of a parent pull request? The first two commits seem fairly mechanical, but the last one less so. The comments in the code are helpful, but some pointers in the PR description for what to look out for, or a broad description of the approaches taken would be helpful too. E.g. why is so much custom functionality introduced for findBlock?
  27. ryanofsky commented at 4:38 pm on July 8, 2024: contributor

    re: #29409 (comment)

    Thanks for the questions.

    I’m not sure what a good approach for reviewing this might be. How can I validate the changes without tests? Should I be running this as part of a parent pull request?

    You could test this as part of #10102, and I should probably add some unit test code to ipc_test.cpp to make sure new runtime code in capnp/chain.cpp and capnp/common-types.h functions has test coverage.

    Most of the code in the PR is evaluated at compile time though, so for example, if there is a mismatch in any of the interface declarations there will be build errors.

    The first two commits seem fairly mechanical, but the last one less so. The comments in the code are helpful, but some pointers in the PR description for what to look out for, or a broad description of the approaches taken would be helpful too.

    This is helpful feedback, and I will try to add more comments. I know it needs more comments, but it’s hard for me to know where to add them without being asked because I’m too familiar with everything here. This PR is a case where “Don’t spend more than a few seconds trying to understand it” review really advice applies, so if something doesn’t make sense, you should just ask so I can clarify. If something doesn’t make sense to you, it probably doesn’t make sense to other people either.

    E.g. why is so much custom functionality introduced for findBlock?

    Will add a comment, but Chain::findBlock and the handful of other Chain methods taking FoundBlock input/output arguments need custom code because these are converted to FoundBlockParam and FoundBlockResult Cap’n Proto structs to make IPC calls, and these structs are recursive. Libmultiprocess has decent support for converting back and forth between C++ classes and capnp structs generally, but it would be hard for it support recursive structs. It also might have a hard time in this case because there is a not 1:1 mapping between fields in the c++ class and capnp structs. So the conversion for these types is completely custom.

    Similar custom code also exists for the C++ BlockInfo struct which is complicated to translate to the Capnp BlockInfo struct, because it has internal pointers and references, so it has custom code because it can’t really be converted automatically.

  28. ryanofsky force-pushed on Jul 26, 2024
  29. in src/ipc/capnp/common-types.h:186 in 198fbd295b outdated 
    206+{
207+    BuildField(TypeList<std::string>(), invoke_context, output, std::string(value.what()));
208+}
209+
210+template <typename Input, typename ReadDest>
211+decltype(auto) CustomReadField(TypeList<UniValue::type_error>, Priority<1>, InvokeContext& invoke_context,
    TheCharlatan commented at 4:19 pm on July 26, 2024:
    Just a question: From what I gathered so far UniValue is required in these interfaces for error handling (like here) and to pass around the ban map. Is that correct? It seems unfortunate to me that UniValue types end up in the interfaces and need to be handled here. Is the intention to eventually refactor this?
    ryanofsky commented at 5:11 pm on July 26, 2024:

    re: #29409 (review)

    Just a question: From what I gathered so far UniValue is required in these interfaces for error handling (like here) and to pass around the ban map. Is that correct? It seems unfortunate to me that UniValue types end up in the interfaces and need to be handled here. Is the intention to eventually refactor this?

    In general there are lots of ways current interfaces are not ideal and can be improved over time, to improve performance (especially to reduce round trips), reduce dependencies, and provide more flexibility.

    On UniValue specifically, handling UniValue exceptions could go away if wallet processes would start their own RPC servers on their own ports instead of getting RPC requests forwarded to them by the bitcoin-node process. That wouldn’t be hard to do, but it would be a UI change, and there’s a chicken and egg problem because it doesn’t really make sense to make wallets listen on different ports before wallets are able to run in separate processes.

    UniValue is also used to pass settings between processes. For example when GUI settings are changed, the GUI calls the updateRwSetting() function which takes a univalue parameter to save the setting. In this case I think it makes sense for the interface to use univalues, since the settings are saved as JSON, though of course alternatives are possible and might be more ideal.

    For the ban map, maybe univalue is not ideal for some reason, but it also might be better than defining a new serialization format. I don’t see a problem with the current approach in any of these cases, but making changes is always possible

  30. ryanofsky commented at 4:34 pm on July 26, 2024: contributor

    Updated ab6b795f218b2074a9c9c05fcc94ec37eccca5a5 -> eec0d31850aa1f7c520a46c417320f480f23fb1c (pr/ipc-chain.5 -> pr/ipc-chain.6, compare), rebasing and dropping no longer needed code after libmultiprocess update, adding comments and tests, and splitting up commits. There is still more to do to splitting up commits and adding tests, however.

    This PR now shares a little bit of code with #30510, so will get smaller if #30510 is merged. #30510 is also simpler and should be easier to review, so will mark this as a draft, although feedback is still welcome on this PR especially if there are questions or things that need more explanation.

    re: #29409#pullrequestreview-1921347149

    being able to work with any socket (TCP or UNIX) makes a lot of sense. you could have bitcoin-node on some secure computing host. and then one or more bitcoin-walet on over-the-network hosts

    Yes this PR does not actually expose a socket but #30509 does, and #19460 uses it for the wallet (and #19461 uses it for the gui, and https://github.com/Sjors/bitcoin/pull/48 and #30437 use it for the mining interface

  31. ryanofsky marked this as a draft on Jul 26, 2024
  32. ryanofsky force-pushed on Jul 26, 2024
  33. DrahtBot added the label CI failed on Jul 26, 2024
  34. DrahtBot commented at 5:17 pm on July 26, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27972388234

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  35. ryanofsky removed the label CI failed on Aug 6, 2024
  36. josibake commented at 10:40 am on August 7, 2024: member
    Small nit: can you update the description with what you mention in #29409#pullrequestreview-2202254574 , regarding #30509 ? I was surprised to see this PR in draft and it took me a bit of digging in the comments to learn why. It’s more clear when reading the “process separation” issue, but I think it would also be helpful to mention in this PR description.
  37. ryanofsky commented at 2:43 pm on August 7, 2024: contributor

    re: #29409 (comment)

    Thanks! Added following to the description:

    Note: The changes in this PR allow a new bitcoin-node process to be started with an -ipcfd=<n> argument specifying a file descriptor from a socketpair, allowing another process to control it over the socketpair. This PR by itself is fairly inflexible but combined with #30509 it allows existing bitcoin-node processes to be controlled over a unix socket. This PR is also a dependency of #10102 which allows the wallet code to run in a separate process and use the Chain interface.

  38. hebasto added the label Needs CMake port on Aug 16, 2024
  39. DrahtBot added the label Needs rebase on Sep 2, 2024
  40. maflcko removed the label Needs CMake port on Sep 3, 2024
  41. achow101 referenced this in commit df3f63ccfa on Sep 9, 2024
  42. ryanofsky force-pushed on Sep 19, 2024
  43. ryanofsky commented at 10:56 am on September 19, 2024: contributor
    Rebased 43dc39eed47d83b2b7e72c911198bbdd401c78d8 -> 65c4edda94ead5c20969681f8337fd6a735182cc (pr/ipc-chain.7 -> pr/ipc-chain.8, compare) due to conflicts with #30697 and #30454 Rebased 65c4edda94ead5c20969681f8337fd6a735182cc -> 968f14fc7a634f65f4399dc042a37d4a39f2c703 (pr/ipc-chain.8 -> pr/ipc-chain.9, compare) after base PR #30510 merged
  44. DrahtBot removed the label Needs rebase on Sep 19, 2024
  45. DrahtBot added the label Needs rebase on Sep 25, 2024
  46. Add capnp serialization code for bitcoin types 
    - Add capnp ToBlob, ToArray, Wrap, Serialize, and Unserialize helper functions
- Add support for std::chrono::seconds capnp serialization
- Add support for util::Result capnp serialization
    e7619a84bd
  47. Add capnp wrapper for Handler interface 76fd4e56c4
  48. Add capnp wrapper for Chain interface f6c7786c41
  49. multiprocess: Expose Chain interface 
    Expose Chain interface to external processes spawning or connecting to
bitcoin-node.
    64b833854a
  50. ryanofsky force-pushed on Sep 26, 2024
  51. ryanofsky marked this as ready for review on Sep 26, 2024
  52. DrahtBot removed the label Needs rebase on Sep 26, 2024
  53. josibake commented at 5:16 pm on September 26, 2024: member

    Concept ACK

    Thanks for the rebase, I’ll be digging into this next week!

  54. in src/ipc/capnp/chain.capnp:23 in 968f14fc7a outdated 
    18+interface Chain $Proxy.wrap("interfaces::Chain") {
19+    destroy @0 (context :Proxy.Context) -> ();
20+    getHeight @1 (context :Proxy.Context) -> (result :Int32, hasResult :Bool);
21+    getBlockHash @2 (context :Proxy.Context, height :Int32) -> (result :Data);
22+    haveBlockOnDisk @3 (context :Proxy.Context, height :Int32) -> (result :Bool);
23+    getTipLocator @4 (context :Proxy.Context) -> (result :Data);
    TheCharlatan commented at 12:17 pm on November 8, 2024:

    Looking at the usage of this endpoint I am wondering if it is even necessary? I.e. instead of getting it first and using it for chainStateFlushed, the chainStateFlushed implementation could just do that internally.

    More generally I have been wondering if it would be a good idea to eventually strongly type these Data blobs (and similarly the enums)? For example here, wrapping it in a BlockLocator type. I don’t think this is relevant as long as libmultiprocess is used to create the c++ wrappers, which do have the full data types, but I am not sure how other languages would do this if they were used as clients to the interfaces eventually. Do you know how this could look like?

    ryanofsky commented at 1:55 pm on November 12, 2024:

    re: #29409 (review)

    Looking at the usage of this endpoint I am wondering if it is even necessary? I.e. instead of getting it first and using it for chainStateFlushed, the chainStateFlushed implementation could just do that internally.

    Current usages are a little unusual because chainStateFlushed() is used in the wallet both to receive notifications from the node about when to flush, but also internally in the wallet to flush its own data. I’m not sure if I understand the exact code change you are suggesting, but it could be reasonable and I also have a PR #29652 which removes getTipLocator and getActiveChainLocator.

    More generally I have been wondering if it would be a good idea to eventually strongly type these Data blobs (and similarly the enums)? […]

    Yes all that makes sense. The tradeoff is just that defining capnproto structs for all the serialized classes adds more code that will need to be maintained, so I think it should probably be done selectively whenever use cases arise, probably by adding new versions of methods using new types. These changes can be implemented incrementally as needed, instead of all up front.

  55. in src/ipc/capnp/chain.capnp:37 in 968f14fc7a outdated 
    32+    findCommonAncestor @13 (context :Proxy.Context, blockHash1 :Data, blockHash2 :Data, ancestor :FoundBlockParam, block1 :FoundBlockParam, block2 :FoundBlockParam) -> (ancestor :FoundBlockResult, block1 :FoundBlockResult, block2 :FoundBlockResult, result :Bool);
33+    findCoins @14 (context :Proxy.Context, coins :List(Common.Pair(Data, Data))) -> (coins :List(Common.Pair(Data, Data)));
34+    guessVerificationProgress @15 (context :Proxy.Context, blockHash :Data) -> (result :Float64);
35+    hasBlocks @16 (context :Proxy.Context, blockHash :Data, minHeight :Int32, maxHeight: Int32, hasMaxHeight :Bool) -> (result :Bool);
36+    isRBFOptIn @17 (context :Proxy.Context, tx :Data) -> (result :Int32);
37+    isInMempool @18 (context :Proxy.Context, tx :Data) -> (result :Bool);
    TheCharlatan commented at 12:43 pm on November 8, 2024:
    Nit: Shouldn’t this be called txid?
    ryanofsky commented at 1:59 pm on November 12, 2024:

    re: #29409 (review)

    Nit: Shouldn’t this be called txid?

    Nice catch, fixed! Might be a good idea to make sure these names are correct, even though they are just documentation and don’t affect behavior.

  56. in src/ipc/capnp/chain.capnp:68 in 968f14fc7a outdated 
    63+    rpcRunLater @44 (context :Proxy.Context, name :Text, fn: RunLaterCallback, seconds: Int64) -> ();
64+    getSetting @45 (context :Proxy.Context, name :Text) -> (result :Text);
65+    getSettingsList @46 (context :Proxy.Context, name :Text) -> (result :List(Text));
66+    getRwSetting @47 (context :Proxy.Context, name :Text) -> (result :Text);
67+    updateRwSetting @48 (context :Proxy.Context, name :Text, update: SettingsUpdateCallback) -> (result :Bool);
68+    overwriteRwSetting @49 (context :Proxy.Context, name :Text, value :Text, write :Bool) -> (result :Bool);
    TheCharlatan commented at 12:54 pm on November 8, 2024:
    How does this write argument here map to the action argument in the c++ interface? They seem to have different types.
    ryanofsky commented at 3:58 pm on November 12, 2024:

    re: #29409 (review)

    How does this write argument here map to the action argument in the c++ interface? They seem to have different types.

    This is a bug, and it should have been caught by the libmultiprocess library. https://github.com/chaincodelabs/libmultiprocess/pull/120 adds a static_assert to ensure that it would result in a compile error if a specified capnproto type is ever not compatible with a c++ enum type.

  57. in src/ipc/capnp/chain.capnp:78 in 968f14fc7a outdated 
    73+
74+interface ChainNotifications $Proxy.wrap("interfaces::Chain::Notifications") {
75+    destroy @0 (context :Proxy.Context) -> ();
76+    transactionAddedToMempool @1 (context :Proxy.Context, tx :Data) -> ();
77+    transactionRemovedFromMempool @2 (context :Proxy.Context, tx :Data, reason :Int32) -> ();
78+    blockConnected @3 (context :Proxy.Context, role: UInt32, block :BlockInfo) -> ();
    TheCharlatan commented at 12:57 pm on November 8, 2024:
    Why is the role typed as UInt32 when the other enums are typed as Int32?
    ryanofsky commented at 4:02 pm on November 12, 2024:

    re: #29409 (review)

    Why is the role typed as UInt32 when the other enums are typed as Int32?

    Now changed this to Int32 to be consistent. Probably UInt32 was used because this parameter was added at a later time than all the other code was written. The exact integer type used to represent the enum doesn’t actually matter as long as it is wide enough to hold all the enum values, and as long as round-trip static casts from the enum type to the int type and back return the same enum value. After https://github.com/chaincodelabs/libmultiprocess/pull/120 it should also be a compile error if incompatible types are used

  58. in src/ipc/capnp/chain.capnp:144 in 968f14fc7a outdated 
    139+    id @0 :Text;
140+    method @1 :Text $Proxy.name("strMethod");
141+    params @2 :Text;
142+    mode @3 :Int32;
143+    uri @4 :Text $Proxy.name("URI");
144+    authUser @5 :Text;
    TheCharlatan commented at 1:05 pm on November 8, 2024:
    Should this include the version as well at this point?
    ryanofsky commented at 4:05 pm on November 12, 2024:

    re: #29409 (review)

    Should this include the version as well at this point?

    Yes, good catch. Added the version field and added a comment to JSONRPCRequest struct to prevent this type of bug in the future. I think this bug did not matter too much in practice because this capnp struct is only used to forward RPC requests from the node to the wallet and wallet shouldn’t respond to requests differently based on JSONRPC version, but it is still a bug in the serialization that should be fixed.

  59. ryanofsky force-pushed on Nov 12, 2024
  60. ryanofsky commented at 4:27 pm on November 12, 2024: contributor

    Updated 968f14fc7a634f65f4399dc042a37d4a39f2c703 -> 34d3e2a6eaaab9de5328c3e64739f1392696c7db (pr/ipc-chain.9 -> pr/ipc-chain.10, compare) with review suggestions and fixes.

    Thanks for the review!

  61. TheCharlatan approved
  62. TheCharlatan commented at 11:47 am on November 19, 2024: contributor
    lgtm ACK 34d3e2a6eaaab9de5328c3e64739f1392696c7db
  63. DrahtBot requested review from josibake on Nov 19, 2024
  64. ryanofsky commented at 8:33 pm on December 4, 2024: contributor
    Added 1 commit 34d3e2a6eaaab9de5328c3e64739f1392696c7db -> 395d5eed0475558de2c2a66914d5a5814ce38f3c (pr/ipc-chain.10 -> pr/ipc-chain.11, compare) exposing the Chain interface so it is easier to experiment with. Updated 395d5eed0475558de2c2a66914d5a5814ce38f3c -> 64b833854a34d87cde4e0ca4173d75012c401a7a (pr/ipc-chain.11 -> pr/ipc-chain.12, compare) fixing FoundBlock locator serialization to fix wallet_backup.py --descriptors test failure in followup #10102 https://cirrus-ci.com/task/6269118947524608?logs=ci#L2529 in new test added #30678
  65. in src/ipc/capnp/chain.capnp:80 in 395d5eed04 outdated 
    75+    destroy @0 (context :Proxy.Context) -> ();
76+    transactionAddedToMempool @1 (context :Proxy.Context, tx :Data) -> ();
77+    transactionRemovedFromMempool @2 (context :Proxy.Context, tx :Data, reason :Int32) -> ();
78+    blockConnected @3 (context :Proxy.Context, role: UInt32, block :BlockInfo) -> ();
79+    blockDisconnected @4 (context :Proxy.Context, block :BlockInfo) -> ();
80+    updatedBlockTip @5 (context :Proxy.Context) -> ();
    darosior commented at 2:50 pm on December 5, 2024:
    What is the use for an updatedBlockTip notification with no information about the new tip?
    Tarhanshop commented at 8:39 pm on December 6, 2024:
    Pkg install
    TheCharlatan commented at 10:10 pm on December 9, 2024:
    Adding this here for context: I guess one of the benefits of the Notifications and NotificationsProxy is avoiding raw pointer types in these interfaces. I went through the exercise of removing the pointer types from the validation interface before. Maybe this could eliminate the need for the logic in the NotificationsProxy in the first place. Not having to handle pointer types is also useful for the kernel API, where conveying the semantics of these pointers in the notifications to the using developer is tricky.
  66. ryanofsky force-pushed on Dec 6, 2024
  67. darosior commented at 7:58 pm on December 6, 2024: member
    In order to test this PR i implemented a PoC of a Rust wallet (using BDK) consuming the Chain interface introduced here. I think it’s also a good example of what multiprocess enables. See https://github.com/darosior/core_bdk_wallet.
  68. in src/ipc/capnp/chain.capnp:42 in 64b833854a 
    37+    isInMempool @18 (context :Proxy.Context, txid :Data) -> (result :Bool);
38+    hasDescendantsInMempool @19 (context :Proxy.Context, txid :Data) -> (result :Bool);
39+    broadcastTransaction @20 (context :Proxy.Context, tx: Data, maxTxFee :Int64, relay :Bool) -> (error: Text, result :Bool);
40+    getTransactionAncestry @21 (context :Proxy.Context, txid :Data) -> (ancestors :UInt64, descendants :UInt64, ancestorsize :UInt64, ancestorfees :Int64);
41+    calculateIndividualBumpFees @22 (context :Proxy.Context, outpoints :List(Data), targetFeerate :Data) -> (result: List(Common.PairInt64(Data)));
42+    calculateCombinedBumpFee @23 (context :Proxy.Context, outpoints :List(Data), targetFeerate :Data) -> (result :Int64, hasResult :Bool);
    darosior commented at 9:18 pm on December 9, 2024:
    Why are the CFeeRate serialized as Data whereas the other CAmount are Int64?
  69. darosior commented at 10:12 pm on December 9, 2024: member

    tested ACK 395d5eed0475558de2c2a66914d5a5814ce38f3c

    I’ve tested this with the Rust wallet PoC i mentioned earlier: #29409 (comment). This tool exercises the following methods: getHeight, getBlockHash, hasBlocks, findAncestorByHeight, findAncestorByHash, findCommonAncestor, initMessage, showProgress and handleNotifications. It also exercises all the ChainNotifications events.

    I tested numerous scenarii with this tool on regtest (some are documented here). I then proceeded to scan some existing Signet wallets while syncing a Signet node, as well as performing a rescan on a synced Signet node. I did not find any issue with this PR in all those tests (no crash, i get accurate wallet information and events under various conditions). I found a crash in #10102, but it’s specific to the new logic introduced there and i could not reproduce it using this PR.

    I cursory-reviewed the rest of the interface not exercised in my Rust wallet and did not find any issue.

  70. TheCharlatan approved
  71. TheCharlatan commented at 10:25 pm on December 9, 2024: contributor

    Re-ACK 64b833854a34d87cde4e0ca4173d75012c401a7a

    fixing FoundBlock locator serialization to fix wallet_backup.py –descriptors test failure in followup

    I should have seen that one though :| - good that those tests exist!

  72. DrahtBot requested review from darosior on Dec 9, 2024
  73. darosior commented at 4:30 pm on December 16, 2024: member

    I found another segmentation fault in updating my previous ACK to the latest tip of this PR. Here are minimal steps to reproduce:

    1. Run a bitcoin-node from this PR. I tested on regtest with: ./multiprocbuild/src/bitcoin-node -regtest -datadir=$PWD/datadir_bdk_wallet -server=0 -ipcbind=unix -debug=ipc
    2. Have a client connect. I used my tool for this. See usage here. The command i ran: cargo run -- ../bitcoin/datadir_bdk_wallet/regtest/node.sock.
    3. Stop the bitcoin-node process. It will hang waiting for the client to exit.
    4. Stop the client. The bitcoin-node process will segfault.

    Here are the debug logs of bitcoin-node from when the client connected to the segfault.

     02024-12-16T16:19:22Z Loading 1 mempool transactions from file...
 12024-12-16T16:19:22Z Loading addresses from DNS seed dummySeed.invalid.
 22024-12-16T16:19:22Z Imported mempool transactions from file: 1 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
 32024-12-16T16:19:22Z initload thread exit                                                                 
 42024-12-16T16:19:22Z 0 addresses found from DNS seeds
 52024-12-16T16:19:22Z dnsseed thread exit                                                                  
 62024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#1](/bitcoin-bitcoin/1/) Init.construct$Params ()
 72024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#1](/bitcoin-bitcoin/1/) Init.construct$Results (threadMap = <external capability>)
 82024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#2](/bitcoin-bitcoin/2/) Init.makeChain$Params (context = (thread = <external capability>))
 92024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server post request  [#2](/bitcoin-bitcoin/2/) {bitcoin-node-8814/b-capnp-loop-8842 (from )}
102024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#2](/bitcoin-bitcoin/2/) Init.makeChain$Results (result = <external capability>)
112024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#3](/bitcoin-bitcoin/3/) Chain.initMessage$Params (context = (thread = <external capability>), message = "Oxydation of the Bitcoin Core wallet in progress..")
122024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server post request  [#3](/bitcoin-bitcoin/3/) {bitcoin-node-8814/b-capnp-loop-8842 (from )}
132024-12-16T16:19:26Z init message: Oxydation of the Bitcoin Core wallet in progress..
142024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#3](/bitcoin-bitcoin/3/) Chain.initMessage$Results ()
152024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#4](/bitcoin-bitcoin/4/) Chain.showProgress$Params (context = (thread = <external capability>), title = "BDK Core startup", progress = 1, resumePossible = false)
162024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server post request  [#4](/bitcoin-bitcoin/4/) {bitcoin-node-8814/b-capnp-loop-8842 (from )}
172024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#4](/bitcoin-bitcoin/4/) Chain.showProgress$Results ()
182024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#5](/bitcoin-bitcoin/5/) Chain.getHeight$Params (context = (thread = <external capability>))
192024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server post request  [#5](/bitcoin-bitcoin/5/) {bitcoin-node-8814/b-capnp-loop-8842 (from )}
202024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#5](/bitcoin-bitcoin/5/) Chain.getHeight$Results (result = 113, hasResult = true)
212024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#6](/bitcoin-bitcoin/6/) Chain.getBlockHash$Params (context = (thread = <external capability>), height = 113)
222024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server post request  [#6](/bitcoin-bitcoin/6/) {bitcoin-node-8814/b-capnp-loop-8842 (from )}
232024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#6](/bitcoin-bitcoin/6/) Chain.getBlockHash$Results (result = "H\\3678\\307#\\020|\\020\\017\\340\\2469\\330\\356\\335\\277\\341dOB{\\220\\327\\354[\\361\\036\\233\\301\\035DD")
242024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#7](/bitcoin-bitcoin/7/) Chain.showProgress$Params (context = (thread = <external capability>), title = "BDK Core startup", progress = 100, resumePossible = true)
252024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server post request  [#7](/bitcoin-bitcoin/7/) {bitcoin-node-8814/b-capnp-loop-8842 (from )}
262024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#7](/bitcoin-bitcoin/7/) Chain.showProgress$Results ()
272024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#8](/bitcoin-bitcoin/8/) Chain.handleNotifications$Params (context = (thread = <external capability>), notifications = <external capability>)
282024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server post request  [#8](/bitcoin-bitcoin/8/) {bitcoin-node-8814/b-capnp-loop-8842 (from )}
292024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#8](/bitcoin-bitcoin/8/) Chain.handleNotifications$Results (result = <external capability>)
302024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server destroy N2mp11ProxyServerIN3ipc5capnp8messages7HandlerEEE
31^C2024-12-16T16:19:27Z tor: Thread interrupt                                                              
322024-12-16T16:19:27Z Shutdown: In progress...                                                             
332024-12-16T16:19:27Z addcon thread exit                                                                   
342024-12-16T16:19:27Z opencon thread exit                                                                  
352024-12-16T16:19:27Z torcontrol thread exit                                                               
362024-12-16T16:19:27Z net thread exit                                                                      
372024-12-16T16:19:27Z msghand thread exit                                                                  
382024-12-16T16:19:27Z DumpAnchors: Flush 0 outbound block-relay-only peer addresses to anchors.dat started
392024-12-16T16:19:27Z DumpAnchors: Flush 0 outbound block-relay-only peer addresses to anchors.dat completed (0.00s)
402024-12-16T16:19:27Z scheduler thread exit                                                                
412024-12-16T16:19:27Z Writing 1 mempool transactions to file...
422024-12-16T16:19:27Z Writing 0 unbroadcast transactions to file.
432024-12-16T16:19:27Z Dumped mempool: 0.000s to copy, 0.002s to dump, 289 bytes dumped to file
442024-12-16T16:19:27Z Flushed fee estimates to fee_estimates.dat.
452024-12-16T16:19:27Z [ipc] {bitcoin-node-8814/b-shutoff-8814} IPC client first request from current thread, constructing waiter
462024-12-16T16:19:27Z [ipc] {bitcoin-node-8814/b-shutoff-8814} IPC client send ChainNotifications.chainStateFlushed$Params (context = (thread = <external capability>, callbackThread = <external capability>), role = 0, locator = "\\200\\021\\001\\000\\022H\\3678\\307#\\020|\\020\\017\\340\\2469\\330\\356\\335\\277\\341dOB{\\220\\327\\354[\\361\\036\\233\\301\\035DD\\tGm\\237]YbZDV9\\254\\252f\\205\\001\\323K\\375!\\340\\t\\367\\375\\346\\264W\\207\\000\\2101\\035:\\314\\233\\246\\275\\207\\v\\\\\\\\\\343\\202\\026\\274\\037\\213\\314\\303V\\2116<*+\\352\\255\\026\\204M\\332\\340\\nT\\306\\360Pa\\304\\002\\024\\356\\247\\220\\263\\257l\\230\\350\\362\\027Yl\\300\\316lk\\232\\f\\333\\235,\\234\\336\\204+JE\\263>\\237\\177\\305z\\017\\252\\311@\\341e\\026\\300dk\\305\\212V\\334\\377\\004cb\\217p\\n\\307w\\0046\\2261|\\024\\\\N\\336\\223G\\263~n\\271\\256\\264\\346\\227\\301\\032\\301\\030\\bw\\241Z.\\302\\261\\027 \\005\\374B\\357R\\231\\237Ct\\324\\232I\\322\\370\\303T\\253\\205\\354\\233\\337\\031C\\3215O\\3746\\016\\360b &\\300\\201\\244U\\221\\306 Pql+c\\212\\2524\\314a\\t\\216\\275=\\230b\\207ouv[\\365(\\3...
472024-12-16T16:19:27Z [ipc] {bitcoin-node-8814/b-shutoff-8814} IPC client recv ChainNotifications.chainStateFlushed$Results ()
482024-12-16T16:19:27Z Shutdown: done                                                                       
492024-12-16T16:19:32Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server destroy N2mp11ProxyServerIN3ipc5capnp8messages5ChainEEE
502024-12-16T16:19:32Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server: socket disconnected.
512024-12-16T16:19:32Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server destroy N2mp11ProxyServerIN3ipc5capnp8messages4InitEEE
52Segmentation fault


ryanofsky DrahtBot cbergqvist ariard TheCharlatan josibake darosior Tarhanshop


josibake darosior

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