[wip,sketch] libbitcoin_net #47
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit introduces libbitcoin_net. A single header connection_types.h and its implementation are moved there to ensure all scaffolding is in place.
The typedef int64_t NodeId is used in several places which generally have no need for anything else in net.h. Specifically the introduction of net/util.h prevents a circular dependency in the next commit.
Moving MAX_PROTOCOL_MESSAGE_LENGTH out of net.h is needed to prevent a circular dependency in the upcoming commit that moves Transport to Net.
This commit also adds seperate header and implementation files for TransportV1 and TransportV2.
This removes the Transport dependency on chainparams.h (libbitcoin_common).
libbitcoin_wallet has no dependency on libbitcoin_net. Drop the unused include so the next commit can move protocol.h to libbitcoin_net without adding such a dependency.
GenerateRandomKey() is used by V2Transport to generate the ephemeral keys used in the handshake.
The ci "test-each-commit" job fetches the PR branch being tested with a depth of (# of commits in PR + 2), and then tries to run tests on commits after the most recent merge commit. When a PR is opened against a bitcoin core branch, a merge commit is always guaranteed to exist within the fetch depth, because bitcoin core branches always point at merge commits. However, in fork repositories, pull requests can be opened based on other branches that don't contain recent merge commits, and this will currently cause git rev-list to fail with fatal: bad revision '^^@'. Work around this problem by not requiring a recent merge commit, and just testing on all fetched commits if a merge commit can't be found. Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
b64fc46 to
6088dd7
Compare
|
Regarding he Win64 build failures, it's probably easier to wait for cmake than to figure out how to add a new library to the various |
This avoids a dependency from util on consensus for BIP32Hash.
f348006 to
0d4b40e
Compare
Moving ExtPubKey out of consensus avoids a dependency from consensus on util for BIP32Hash. Move the CPubKey::Derive implementation into ExtPubKey::Derive to avoid a dependency of CPubKey on BIP32Hash.
f67b0b6 to
8f2566b
Compare
This was referenced Jun 28, 2024
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce
libbitcoin_netfor all networking stuff.See libraries.md for the current architecture.
This PR probably involves too many changes for something that doesn't solve an acute problem. But it's hopefully an interesting sketch and could inform discussions on further library design (and where to put stuff).
My own goal is to end up with
libbitcoin_sv2which only depends onlibbitcoin_net,libbitcoin_cryptoandlibbitcoin_util. I might however settle for just a dependency onlibbitcoin_common(as opposed to the current situation where the Template Provider is part oflibbitcoin_nodeand has various dependencies on it).Having networking code in its own library may have additional benefits:
Transportbitcoin-walletandbitcoin-util)The main changes are:
libbitcoin_net. Moving it out oflibbitcoin_nodeis what got me down this rabbit hole; it's fairly easy to move it tolibbitcoin_common, but I wanted to see if I could go beyond that.libbitcoin_net:CNetMessage,NodeID,MAX_PROTOCOL_MESSAGE_LENGTH, BIP324, etc.Params().MessageStart()into Transport so it doesn't needlibbitcoin_commonforchainparams.h.key.hfromlibbitcoin_commontolibbitcoin_util(forGenerateRandomKey())libbitcoin_util(mainlyBIP32Hash)ExtPubKeyandEllSwifttolibbitcoin_util(pubkey.his part oflibbitcoin_consensuswhich I'm trying to avoid a dependency on)Some observations:
libbitcoin_cryptois not ideal (and I didn't do so) because it would bloat the kernel (the one thing we plan to have a stable API for and want to keep lean and mean).XOnlyPubKey(and maybe a few other things) which are needed for consensus, but also for ECDH (transport v2, sv2 noise) and signing (e.g. an sv2 certificate). This means they can't be inlibbitcoin_util, becauselibbitcoin_consensuscan't have dependency on that. See circular dependency below.TODO:
contrib/devtools/check-deps.shXOnlyPubKey: part of both consensus and kernel, but (after my refactor) needed by util forCKey::SignSchnorr.libbitcoin_crypto, but that seems the wrong placeSignSchnorrtolibbitcoin_commonEllSwiftPubKey: it's used bykey.cppwhich is in util. I could move all ofkey.cppto common, but then Net would depend on common.CPubKey::Derive->CExtPubKey::Derivemovelibbitcoin_net-->libbitcoin_consensuslibbitcoin_net-->libbitcoin_kernel