#26697 (logging: use bitset for categories by LarryRuane)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
DrahtBot added the label
CI failed
on Feb 14, 2024
DrahtBot
commented at 12:27 pm on February 14, 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.
For the time being I just added handling for MSG_MORE (on e.g. macOS sequential messages are sent separately while on Linux they’re combined). I also made the timeouts a bit longer.
Hopefully that does the trick. This can be revisited closer to the time when the Template Provider is ready for its own PR.
bee6bdf0a5 test: put the generic parts from StaticContentsSock into a separate classf42e4f3b3btest: add a mocked Sock that allows inspecting what has been Send() to it
and then how to use that in the last commit of the same PR:
8b10990aa0 test: add unit tests exercising full call chain of CConnman and PeerManager
With those it is possible to send/receive raw bytes to/from the (mocked) socket, or NetMsgs, e.g.:
TSAN is tripping up somewhere around here. The last thing it logs is - Connect 2 transactions:. It doesn’t get to - Verify ... txins:. I wonder if this is related to mock time, which I’m testing in https://github.com/Sjors/bitcoin/pull/34
When running this locally on Ubuntu with clang 16.0.6 I get a WARNING: ThreadSanitizer: data race and significantly more details (still a bit cryptic, but hopefully enough to figure out what’s happening).
maflcko
commented at 11:39 am on February 15, 2024:
Ah, I guess the unit tests don’t capture the tsan output?
maflcko
commented at 11:41 am on February 15, 2024:
maflcko
commented at 11:42 am on February 15, 2024:
Maybe good to re-check this when/after the cmake migration is done?
Sjors force-pushed
on Feb 15, 2024
Sjors force-pushed
on Feb 15, 2024
Sjors
commented at 2:07 pm on February 15, 2024:
member
Added m_tp_mutex to Sv2TemplateProvider.
Sjors
commented at 3:12 pm on February 15, 2024:
member
Bumping macOS to 14 on the CI does not help (tried in https://github.com/Sjors/bitcoin/pull/35). I also can’t reproduce this failure on my own Intel macOS machines, not on 13.6.4 and not on 14.2.1. A Sock mock is probably the most robust solution, but it’d be nice to find another workaround.
Another option to consider is using the functional test framework instead, since these are not really unit tests. However that involves implementing the sv2 noise protocol in Python and a bunch of other work to export transport functions to the functional test framework. If anyone feels up to that challenge, let me know…
Sjors force-pushed
on Feb 15, 2024
DrahtBot removed the label
CI failed
on Feb 15, 2024
Sjors force-pushed
on Feb 16, 2024
Sjors
commented at 1:05 pm on February 16, 2024:
member
Added m_clients_mutex similar to the one in Connman. Fixed a deadlock on g_best_block_mutex when we receive a fresh block simultaneously from a peer and a client (SubmitSolution message).
Sjors
commented at 2:28 pm on February 16, 2024:
member
Windows test fails with:
0test/sv2_noise_tests.cpp(62): fatal error: in "sv2_noise_tests/certificate_test": critical check !alice_certificate.Validate(XOnlyPubKey(alice_authority_key.GetPubKey())) has failed
DrahtBot added the label
CI failed
on Feb 16, 2024
DrahtBot
commented at 2:46 pm on February 16, 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.
DrahtBot removed the label
CI failed
on Feb 16, 2024
DrahtBot added the label
CI failed
on Feb 29, 2024
DrahtBot
commented at 8:21 pm on February 29, 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.
Sjors
commented at 11:52 am on March 1, 2024:
member
Rebased and hopefully satisfied new bitcoin-config.h linter rule.
The kind of incidental failure in sv2_template_provider_tests can probably be avoided with the mock socket introduced / improved in ##26812.
DrahtBot removed the label
CI failed
on Mar 1, 2024
DrahtBot added the label
CI failed
on Mar 17, 2024
DrahtBot
commented at 6:23 am on March 17, 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.
Sjors
commented at 8:01 pm on March 18, 2024:
member
Rebased after #27375. Changed the default binding from 0.0.0.0 to 127.0.0.1 to make it abundantly clear that it’s currently unsafe to expose this to the big bad internet. The previous behaviour can be achieved with the new -sv2bind=0.0.0.0
Finally, we immediately listen for new client connections. Previously we’d wait for IBD to finish. However we hold off on sending template updates. Except for Signet, because you may be the only miner and after a day of not mining the chain would get stuck.
DrahtBot removed the label
CI failed
on Mar 19, 2024
DrahtBot added the label
Needs rebase
on Mar 20, 2024
Sjors force-pushed
on Mar 22, 2024
DrahtBot removed the label
Needs rebase
on Mar 22, 2024
DrahtBot added the label
Needs rebase
on Apr 2, 2024
Sjors force-pushed
on Apr 5, 2024
DrahtBot removed the label
Needs rebase
on Apr 5, 2024
DrahtBot added the label
CI failed
on Apr 5, 2024
DrahtBot
commented at 11:17 pm on April 5, 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.
(ignoring CI failure, as I mentioned above I plan on making the tests more robust after #26812)
Sjors referenced this in commit
2125fc5616
on May 3, 2024
DrahtBot added the label
Needs rebase
on May 15, 2024
Sjors force-pushed
on May 30, 2024
Sjors
commented at 9:20 am on May 30, 2024:
member
I dropped 3559085dcc9ee687c347383a0c2a63f3ea16f001 and 3f4c1d1e515a840c4b119e3ba65a8391d4fc7f6b which introduced common/transport.h. It’s difficult to maintain such a large diff and it’s currently causing a link error. Here’s a branch with both of them: https://github.com/Sjors/bitcoin/tree/2024/05/transport
I’d still like to move Transport out of libbitcoin_node into libbitcoin_common, but this is probably better done in a parallel PR.
At the request of people testing Stratum v2 I’m adding testnet4 to this branch, but bear in mind it’s not final.
I still need to do some manual testing on this last push, so be careful.
Sjors force-pushed
on May 30, 2024
DrahtBot removed the label
Needs rebase
on May 30, 2024
Sjors force-pushed
on Jun 7, 2024
Sjors
commented at 2:25 pm on June 7, 2024:
member
Updated to use the interface proposed in #30200. This also fixes a small bug: -blockmintxfee and -blockmaxweight are no longer ignored. When the latter argument is unset, by default blocks have a 4000 byte safety margin, just like with getblocktemplate.
Sjors force-pushed
on Jun 7, 2024
Sjors force-pushed
on Jun 7, 2024
Sjors force-pushed
on Jun 7, 2024
DrahtBot added the label
Needs rebase
on Jun 12, 2024
Sjors force-pushed
on Jun 18, 2024
Sjors
commented at 3:29 pm on June 18, 2024:
member
Rebased, squashed Mining interface usage into the existing commits and fixed the usage of coinbase_output_max_additional_size.
DrahtBot removed the label
Needs rebase
on Jun 18, 2024
Sjors force-pushed
on Jun 18, 2024
Sjors force-pushed
on Jun 18, 2024
Sjors force-pushed
on Jun 18, 2024
Sjors force-pushed
on Jun 20, 2024
Sjors force-pushed
on Jun 20, 2024
Sjors
commented at 1:56 pm on June 20, 2024:
member
I split the commit that introduces sv2_messages.h in two parts:
9fcc95d837dbd20c1003b4c2d3c90b202e0629bd: introduces a single simple message Sv2CoinbaseOutputDataSizeMsg and the plumbing it needs
00d1408cc7cb263fb6e956649a41e0b02e2378aa: adds the other messages
This makes it easier to introduce Sv2Transport in a separate PR, which only needs the first commit.
DrahtBot removed the label
CI failed
on Jun 20, 2024
stickies-v
commented at 1:45 pm on June 21, 2024:
contributor
I’m strongly in favour of having Template Provider (TP) support for SV2, so thank you for your hard work on this @Sjors. I am very concerned about the scope of this approach though, and therefore leaning to an Approach NACK (but open to being persuaded otherwise).
From my reading of the discussion in #27854, it seems the main technical hurdle to just having a separate sv2tpd daemon using bitcoind’s existing interfaces is getblocktemplates having serious limitations as outlined here. That’s a sensible concern, and one we need to address.
Can the getblocktemplate concerns not (at least largely) be addressed by upgrading the existing RPC method, or adding a new one that’s minimal in scope and not SV2 opinionated? I understand this will still be a pretty significant overhaul, but probably still significantly smaller than what is proposed at the moment?
I’m happy to be convinced otherwise, but as it stands, implementing a separate sv2tpd process that uses bitcoinds (minimally) improved interfaces seems like a much more sensible approach to me, and afaict would still address all points listed in https://github.com/Sjors/bitcoin/blob/sv2/doc/stratum-v2.md#advantage-over-getblocktemplate-rpc . I’m curious to hear your thoughts on this.
DrahtBot added the label
Needs rebase
on Jun 21, 2024
Sjors
commented at 4:11 pm on June 21, 2024:
member
upgrading the existing RPC method, or adding a new one that’s minimal in scope and not SV2 opinionated
I don’t think our RPC is suitable for this, because it’s inherently poll based. We want to push out new block templates as soon as something better is found.
Another major change compared to the getblocktemplate RPC is that the TP holds on to templates for a while (currently until after the next block is found, though could be shorter). It needs to do this because solutions for custom block templates have to be submitted by the node that created it. The pool will try, but it might fail if it’s missing transactions.
implementing a separate sv2tpd process
This PR also aims to run in a separate process, eventually anyway. I guess you’re saying it should be separate software altogether, like SRI?
It would move some of the complexity from this PR elsewhere, mainly the encryption and transport layers. It could poll once a second, we’d need changes to hold on to previously generated templates, a new RPC to submit a solution that refers to a previous template and a few other things.
As a practical matter I’m not sure who would be implementing that; SRI is already a pretty complicated codebase with a shortage of developer resources. Some people have been testing this branch in combination with SRI and seem quite happy with it, despite the code being nowhere near merge-ready. But in principle it’s possible.
Sjors force-pushed
on Jun 21, 2024
Sjors
commented at 4:58 pm on June 21, 2024:
member
Rebased after #30314 and #30202. Incorporated the (light) transport refactor from #30315 to make sure that didn’t break anything.
DrahtBot removed the label
Needs rebase
on Jun 21, 2024
stickies-v
commented at 5:48 pm on June 21, 2024:
contributor
I don’t think our RPC is suitable for this, because it’s inherently poll based. We want to push out new block templates as soon as something better is found.
What about a combination of extending the zmq and rest interface and/or using longpollid, as suggested e.g. here? It seems like that would already improve things significantly, without having to add the encryption and transport layer?
It needs to do this because solutions for custom block templates have to be submitted by the node that created it. The pool will try, but it might fail if it’s missing transactions.
I’m not sure I understand this. This PR is not an sv2 discussion, so if my understanding is wrong we don’t need to talk about it here, but are you saying the pool will approve a proposed block template without having/keeping all the transactions? That seems undesired?
Regardless, I’m not sure the TP necessarily needs to hold on to block templates when sv2tpd caches all block template transactions and add any missing ones back to core through RPC before submitting a block? Perhaps the existing interfaces aren’t sufficient currently, but then again - perhaps we could improve them instead of wholesale implementing sv2 logic?
I guess you’re saying it should be separate software altogether
Yes, a separate project and a separate process would be my strong preference. Again, I do think it’s important Bitcoin Core supports SV2 well. But if the bitcoind interfaces are lacking for sv2tpd to be a separate project, I think we should first exhaust the possibilities of improving the interfaces before just incorporating (part of) the downstream project into Bitcoin Core.
As a practical matter I’m not sure who would be implementing that
Wouldn’t it be more straightforward to implement this as a separate sv2tpd daemon versus incorporating it into Core, giving more freedom in terms of language, dependencies, review, release process, …?
sipa
commented at 6:02 pm on June 21, 2024:
member
But if the bitcoind interfaces are lacking for sv2tpd to be a separate project, I think we should first exhaust the possibilities of improving the interfaces before just incorporating (part of) the downstream project into Bitcoin Core.
FWIW, my view is that indeed the current Bitcoin Core interfaces are inadequate for implementing template providers, and while it’s probably possible to make a number of improvements to existing ones to improve the situation, the result will either be far from ideal still (RPC is not binary, ZMQ is only one-directional, …), or quite different from anything we have now. And if we’re going to make a substantially different interface for the purpose of template providing, it probably makes sense to just pick Sv2 to be that interface as it’s designed exactly for that purpose. The encryption aspect of Sv2 isn’t necessary for that, but if all sv2tpd would do is be an Sv2 encrypting proxy, it might be logistically just simpler to have Bitcoin Core also do the encryption aspect.
So this doesn’t exclude the possibility that future block template providing protocols be implemented in external proxy servers, which then talk still talk Sv2 to Bitcoin Core, if that is still appropriate.
Sjors
commented at 6:40 pm on June 21, 2024:
member
I’m not sure I understand this. This PR is not an sv2 discussion, so if my understanding is wrong we don’t need to talk about it here, but are you saying the pool will approve a proposed block template without having/keeping all the transactions? That seems undesired?
The Job Declaration Protocol has a param async_mining_allowed which, if set, allows miners to immediately submit work even before the template has been approved. Afaik SRI currently just hardcodes this to true for simplicity (cc @fi3).
Approval takes time, because we have to send the pool any transactions it’s missing, and it needs to check those (SRI currently only checks that all transactions are present, no additional checks - I suspect that’s a can of worms on its own, since there may be conflicts with the pool node mempool). During this time you would either have to mine on a pool template, or solo mine.
Regardless of the merits of async mining, it’s more robust for the miner to broadcast a found block themselves (this is done via the SubmitSolution message).
Example of this actually going wrong (in testing): #912
I’m not sure the TP necessarily needs to hold on to block templates when sv2tpd caches all block template transactions and add any missing ones back to core through RPC before submitting a block?
Maybe. But we also need to hold on to everything that was in the mempool at the time (an aspect I haven’t thoroughly tested yet). From what I’ve seen in SRI, the Job Declarator Server (pool side) implements some sort of pseudo-mempool for that purpose.
Wouldn’t it be more straightforward to implement this as a separate sv2tpd daemon versus incorporating it into Core
I’m not sure if it is. The two most practical approaches are improving this PR vs. expanding SRI. Building something from scratch is probably more work. Using a language other than Rust of c++ means re-writing the Noise implementation and other reusable code. The Bitcoin Core codebase has some very useful building blocks, e.g. the Transport class, BIP324 implementation (which we use parts of), well tested TCP socket handling code (unfortunately very tightly coupled with p2p, but refactoring that can be orthogonally useful), process separation in the pipeline (so it can crash without taking the node down - though so can a separate tool), good CI, and deterministic builds (so I can easily and relatively safely ship this PR as its own binary).
DrahtBot added the label
CI failed
on Jun 21, 2024
DrahtBot
commented at 8:34 pm on June 21, 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.
DrahtBot added the label
Needs rebase
on Jun 24, 2024
ryanofsky referenced this in commit
323b0acfcb
on Jun 24, 2024
Sjors force-pushed
on Jun 25, 2024
Sjors
commented at 12:18 pm on June 25, 2024:
member
Rebased after #30200 landed! 🎉 Reorganized commits so it’s now based on #30332.
DrahtBot removed the label
Needs rebase
on Jun 25, 2024
stickies-v
commented at 1:43 pm on June 25, 2024:
contributor
…it probably makes sense to just pick Sv2 to be that interface as it’s designed exactly for that purpose
I agree we shouldn’t reinvent the wheel and just implement the parts of sv2 that we need to improve our getblocktemplate functionality. I would just prefer to be much more strict in cutting out all the parts that Core doesn’t require to support sv2 downstream, i.e. I don’t think the “out-of-the-box” aspect should at at all be a priority when that can reasonably be addressed with external tooling.
…it might be logistically just simpler to have Bitcoin Core also do the encryption aspect.
It probably would be, but I’d much rather we keep the scope of Core limited. Where we draw the line is up for debate, but tooling for a highly specialized process (I think we’re only expecting a small minority of nodes to be running SV2?) that can be kept outside of Core absolutely should be, in my view.
…well tested TCP socket handling code (unfortunately very tightly coupled with p2p, but refactoring that can be orthogonally useful), process separation in the pipeline (so it can crash without taking the node down - though so can a separate tool), good CI, and deterministic builds (so I can easily and relatively safely ship this PR as its own binary).
As much as I sympathize that setting up a new project includes a bunch of painful overhead that requires even more scarce developer resources, I don’t think any of these are good reasons to bundle more code than necessary into Core, where it has a massive maintenance cost - even if it’s less visible now.
I think I’m going to withdraw from the conceptual discussion until I have more in-depth knowledge of the relevant code and protocol. To summarize: I think Core should have good support for being a SV2 Template Provider, and implementing parts of SV2 to do that makes sense. It still seems to me like the current approach is too invasive and too much of an unnecessary maintenance burden on this project.
DrahtBot removed the label
CI failed
on Jun 25, 2024
Sjors
commented at 3:05 pm on June 25, 2024:
member
a highly specialized process (I think we’re only expecting a small minority of nodes to be running SV2?)
I’d like to see a lot more users mine, and do so with their own block template. In order to achieve that we have to lower the barrier to entry.
There’s a generic case to be made for reducing the scope of Bitcoin Core, and not further increasing it. I think we still lack the tooling for this. Something like a plugin system, or a convenient way to ship a release tag plus custom functionality.
Having the user download and install two things is fine in theory, as long as it just works(tm). HWI is a working example of that, but that specific approach won’t work here.
I’m not very keen on starting from scratch with a new approach myself, but I can test and review alternative approaches that others work on.
For now I plan to keep rebasing this PR, occasionally releasing a binary. As long as I can get some prerequisite refactoring merged, that should be sustainable for a while.
DrahtBot added the label
Needs rebase
on Jun 27, 2024
Add sv2 log category for Stratum v23f25575ecc
Sjors force-pushed
on Jun 28, 2024
DrahtBot removed the label
Needs rebase
on Jun 28, 2024
Sjors
commented at 9:00 am on June 28, 2024:
member
I opened #30356 with some BlockAssembler::Options and mining interface refactoring that should make this PR smaller.
Add sv2 noise protocol
Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
4778bb7049
Add sv2 message CoinbaseOutputDataSize
This commit adds the simplest stratum v2 message. The remaining messages are introduced in later commits.
Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
1bfa3fbbc6
Convert between Sv2NetMsg and CSerializedNetMsg
This allows us to subclass Transport.
86e6746fdb
Introduce Sv2Transport
Implemented starting from a copy of V2Transport and the V2TransportTester, modifying it to fit Stratum v2 and Noise Protocol requirements.
Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
Co-Authored-By: Fi3
aaad8d116a
Add sv2 SETUP_CONNECTION messages
Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
4ca63a662c
Chainparams: add default sv2 port
Co-authored-by: Christopher Coverdale <chris.coverdale24@gmail.com>
59fbe8e24e
Stratum v2 template provider scaffold
Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
ae14ae4f4a
fixme: make Sv2NetMsg m_msg public
The Template Provider does stuff like this:
DataStream ss (sv2_net_msg.m_msg);
try {
ss >> setup_conn;
} catch (const std::exception& e) {
...
Such error checking should be moved inside Sv2Message.
c64d24238d
Template provider sv2 connection support
The template provider will listen for a Job Declarator client.
It can establish a connection and detect various protocol errors.
Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
Co-Authored-By: Fi3
testnet: Add timewarp attack prevention for Testnet4e2c8451b3e
Persist static key for Template Provider7cd1402179
refactor: add coinbase constraints to options
Stratum v2 requires pools to communicate the maximum bytes they intend to add to the coinbase outputs, as well as the maximum number of sigops.
This commit adds them to BlockAssembler::Options, so they can be set later.
22dd8ff489
Add util/mining.h for default coinbase constraints
These defaults are needed by the Mining interface in the next commit.
a0b696c594
Account for reserved bytes in createNewBlockd9a9aeb6ba
Sv2: construct and submit block templates
Co-Authored-By: Christopher Coverdale <chris.coverdale24@gmail.com>
248d7c6c72
Incrementally update sv2 block template19eefe1d75
Reject RequestTransactionData for stale templates72dd99d2b5
signet: miner skips PSBT step for OP_TRUE392e0e6259
doc: explain Stratum v2 design, testing and usagea10e4dce26
ci: forks can opt-out of CI branch push (Cirrus only)87720f4f77
ci: test-each-commit merge base optional
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>
8f57ab3a98
ci: skip Github CI on branch pushes for forks
Similar to the previous commit, however the behavior isn't opt-in
with NO_BRANCH, because Github CI lacks custom variables.
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-07-01 10:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
