-
Notifications
You must be signed in to change notification settings - Fork 38.8k
signet mining utility #19937
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
signet mining utility #19937
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
435cfff to
e077aa7
Compare
|
EDIT: the following is no longer accurate, see contrib/signet/README.md instead Some examples:
Using a "--block-time=600" will get blocks exactly on 10 minute boundaries, with very little variation. You can alternatively use You can also generate blocks more manually by running:
Using the |
maflcko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a risk in exposing the miner on RPC as well? Bitcoind is running anyway somewhere, and some miners might want to run everything in one process for convenience? No strong opinion.
| } | ||
|
|
||
| char* command = argv[1]; | ||
| if (strcmp(command, "grind") == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not use our ArgsManager here, so that -help documentation will be created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think ArgsManager supports subcommands that have different suboptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would support -grind=block_header_hex. Is something else needed here that I am missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking it would make sense for bitcoin-util would have subcommands like git, each with their own different options (like --amend for git commit vs --continue for git rebase). bitcoin-util grind could have a -jobs=8 argument specifying how many jobs to run in parallel, eg, which presumably wouldn't be appropriate elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wallet tool is doing exactly that already with argsman. E.g. it has the info subcommand, or a dump subcommand with conditional options like -dumpfile (#19137)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems worth addressing still: e6473d2#r492742101
|
I don't think there's a risk in doing it over RPC; it's that it puts the wallet code in the middle of two mining codes bits (ie, generate a template, sign, grind proof of work). Merging mining and wallet code seemed a bit ugly, but keeping them distinct means you need a script to combine them for you, so it was easier to put everything in the script. Also seemed easier to add an RPC later if that makes sense, than to remove it if it doesn't make sense. |
kallewoof
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK e077aa7
|
I still have no idea how to run a custom signet on this PR's commit f4c6ec1 (which has conflicts when I try to rebase it to master locally). The documentation at bitcoin.it is out of date. Please help me so that I can test. |
|
@jsarenik I've updated the running an issuer section of the docs. The rest are fine I think. Lemme know if not. |
|
@kallewoof have a look at #15454 and add the |
|
@kallewoof or start all |
69c6e47 to
f19148a
Compare
contrib/signet/README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MINER="..contrib/signet/miner" > MINER="../contrib/signet/miner"
|
code review ACK f19148a9ddab01af80067982f2639dbee2cdbf52 nit: your new utility doesn't implement |
|
Concept ACK. I think it makes sense to have a bitcoin-util tool with subcommands for wallet- and node-free operations (and deprecate bitcoin-tx in favor of PSBT support in bitcoin-util). |
|
tACK f19148a Few docs nits. |
|
Concept ACK
|
src/rpc/mining.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when signet rules are set when calling getblocktemplate on a non-signet network, I suppose this is (or should be) an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I read BIP9 is that by including "signet" in the rules when doing a GBT request, that just indicates you can handle it if signet rules are active -- if they're not active, then there's no reason why you can't handle that too. So adding signet to rules on mainnet should be fine as far as bitcoind is concerned -- it will just return a regular template without any signet stuff, just as if you'd indicated support for .. extension block mining or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, clear.
f19148a to
595a34d
Compare
|
Rebased to fix the bad library ordering. Suggest reviewing #20715 which is probably the future for bitcoin-util's argument parsing. I think we could update bitcoin-util to work that way either before or after merging this PR. |
|
code review ACK 595a34d |
|
@weedcoder I'd say it is missing a lot of the logic required to do what liquid does, but I do believe this + a permanent elements chain with a 2-way-peg can do a great job as a test-chain for liquid. :) |
…iscv 54ce4fa build: improve macro for testing -latomic requirement (fanquake) 2c010b9 add std::atomic include to bitcoin-util.cpp (fanquake) Pull request description: Since the merge of #19937, riscv builds have been failing, due to a link issue with [`std::atomic_exchange`](https://en.cppreference.com/w/cpp/atomic/atomic_exchange) in `bitcoin-util`: ```bash CXXLD bitcoin-util bitcoin_util-bitcoin-util.o: In function `grind_task': /home/ubuntu/build/bitcoin/distsrc-riscv64-linux-gnu/src/bitcoin-util.cpp:98: undefined reference to `__atomic_exchange_1' collect2: error: ld returned 1 exit status ``` We have a [macro](https://github.com/bitcoin/bitcoin/blob/master/build-aux/m4/l_atomic.m4) that tries to determine when `-latomic` is required, however it doesn't quite work well enough, as it's currently determining it isn't needed: ```bash ./autogen.sh ./configure --prefix=/home/ubuntu/bitcoin/depends/riscv64-linux-gnu ... checking whether std::atomic can be used without link library... yes ``` This PR adds a call to `std::atomic_exchange` to the macro, which will get us properly linked against `-latomic` on riscv: ```bash checking whether std::atomic can be used without link library... no checking whether std::atomic needs -latomic... yes ``` Also adds an `<atomic>` include to `bitcoin-util.cpp`. ACKs for top commit: laanwj: Tested ACK 54ce4fa Tree-SHA512: 963c875097ee96b131163ae8109bcf8fecf4451d20faa2f3d223f9938ea3d8d1ed5604e12ad82c2b4b1c605fd293a9b6b08fefc00dd3e68d09c49e95029c6f50
…g for riscv 54ce4fa build: improve macro for testing -latomic requirement (fanquake) 2c010b9 add std::atomic include to bitcoin-util.cpp (fanquake) Pull request description: Since the merge of bitcoin#19937, riscv builds have been failing, due to a link issue with [`std::atomic_exchange`](https://en.cppreference.com/w/cpp/atomic/atomic_exchange) in `bitcoin-util`: ```bash CXXLD bitcoin-util bitcoin_util-bitcoin-util.o: In function `grind_task': /home/ubuntu/build/bitcoin/distsrc-riscv64-linux-gnu/src/bitcoin-util.cpp:98: undefined reference to `__atomic_exchange_1' collect2: error: ld returned 1 exit status ``` We have a [macro](https://github.com/bitcoin/bitcoin/blob/master/build-aux/m4/l_atomic.m4) that tries to determine when `-latomic` is required, however it doesn't quite work well enough, as it's currently determining it isn't needed: ```bash ./autogen.sh ./configure --prefix=/home/ubuntu/bitcoin/depends/riscv64-linux-gnu ... checking whether std::atomic can be used without link library... yes ``` This PR adds a call to `std::atomic_exchange` to the macro, which will get us properly linked against `-latomic` on riscv: ```bash checking whether std::atomic can be used without link library... no checking whether std::atomic needs -latomic... yes ``` Also adds an `<atomic>` include to `bitcoin-util.cpp`. ACKs for top commit: laanwj: Tested ACK 54ce4fa Tree-SHA512: 963c875097ee96b131163ae8109bcf8fecf4451d20faa2f3d223f9938ea3d8d1ed5604e12ad82c2b4b1c605fd293a9b6b08fefc00dd3e68d09c49e95029c6f50
| #include <stdio.h> | ||
| #include <thread> | ||
|
|
||
| #include <boost/algorithm/string.hpp> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this include is required.
…in-wallet fa61b9d util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke) 7777105 refactor: Move all command dependend checks to ExecuteWalletToolFunc (MarcoFalke) fa06bce test: Add tests (MarcoFalke) fac05cc wallet: [refactor] Pass ArgsManager to WalletAppInit (MarcoFalke) Pull request description: This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937 Fixes: #20902 ACKs for top commit: ajtowns: ACK fa61b9d fjahr: Code review ACK fa61b9d Tree-SHA512: 79622b806e8bf9dcd0dc24a8a6687345710df57720992e83a41cd8d6762a6dc112044ebc58fcf6e8fbf45de29a79b04873c5b8c2494a1eaaf902a2884703e47b
b3c712c contrib/signet/miner: remove debug code (Anthony Towns) 297e351 bitcoin-util: use AddCommand / GetCommand (Anthony Towns) b6d493f contrib/signet/README.md: Update miner description (Anthony Towns) e665438 contrib/signet/miner: Automatic timestamp for first block (Anthony Towns) a383ce5 contrib/signet/miner: --grind-cmd is required for calibrate (Anthony Towns) 1a45cd2 contrib/signet: Fix typos (Anthony Towns) Pull request description: Followups from #19937 ACKs for top commit: laanwj: Code review ACK b3c712c Tree-SHA512: a1003f9ee3697438114b60872b50f4300c8b52f0d58551566eb61c421d787525807ae75be205dcab2c24358cd568f53260120880109a9d728773405ff987596f

Adds
contrib/signet/minerfor mining signet blocks.Adds
bitcoin-utilcli utility, with the idea being it can provide bitcoin related functionality that does not rely on the ability to access a running node. Only subcommand currently is "grind" which takes a hex-encoded header and grinds its nonce until its nBits is satisfied.Updates
getblocktemplateto includesignet_challengefield, and makesgetblocktemplaterequire the signet rule when invoked on the signet change. Removes connectivity and IBD checks fromgetblocktemplatewhen applied to a test chain (regtest, testnet, signet).