signet mining utility #19937

# Copyright (c) 2020 The Bitcoin Core developers

:eyes:

Hmm, think those are leftover from when the code from test_framework/messages.py etc was copied in rather than imported

Any reason to not use our ArgsManager here, so that -help documentation will be created?

I don't think ArgsManager supports subcommands that have different suboptions?

It would support -grind=block_header_hex. Is something else needed here that I am missing?

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.

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)

This seems worth addressing still: https://github.com/bitcoin/bitcoin/pull/19937/commits/e6473d274fe01d58c9eed75bc56415fcce6d63db#r492742101

        std::string strUsage = PACKAGE_NAME " bitcoin-util utility version " + FormatFullVersion() + "\n\n" +

Why is this needed, since there's already --with-util above? (Also, why util-util?)

--with-utils gives the bitcoin-cli, bitcoin-tx, bitcoin-util, bitcoin-wallet utils; --with-util-X just gives bitcoin-X. Not really needed, (and util-util is certainly weird) but don't see any reason to be inconsistent.

operator<= exists, so using that seems more straightforward.

Wouldn't it be totally fine if these simply did exit(...) instead of the EXIT_/CONTINUE_ stuff?

Sure? It's just doing the same thing bitcoin-tx does.

Could cherry-pick the syle-changes from commit faf0a26711eed9264113463e56b988cf9fe549fd?

It would be useful to add a comment for non mining experts, such as yours truly, to explain what this does (especially the ! part, which I guess doesn't mean "not" here). I found the discussion around !segwit rather confusing: https://github.com/bitcoin/bitcoin/pull/17946

Added a comment

Could add missing brackets since you're touching this area.

Done

This is not Python 3.5.6 friendly:

JSON object must be str, not 'bytes'

args.bcli / bitcoin_cli always returns a string now

Dumping the PSBT result is useful here for debugging.

Should dump psbt now if debug logging is enabled. That will contain the whole block as well, so it will be long.

sys.path.insert(0, "../../test/functional/")

in my case, it was wrong path.

cd ./contrib/signet/
./generate.py --help

Yes, the paths are a bit confusing. It is meant to be run from main source root. See for example https://en.bitcoin.it/wiki/Signet:Custom:Script

And the general documentation for Custom Signet is at https://en.bitcoin.it/wiki/Signet#Custom_Signet

Should be fixed

"grind" needs libconsensus for both primitives/block.h for the block header definition and arith_uint256.h for converting nbits to a target and comparing the target to a sha256. Since libconsensus isn't linked into bitcoin-cli, that probably rules out bitcoin-cli --grind=HEADER as an alternative.

Making bitcoin-tx -grind HEADER work seems plausible; should look something like https://github.com/ajtowns/bitcoin/commits/202009-signet-generate-util2tx

Long term I think bitcoin-util is a good idea, but it might be better to introduce it in its own PR, in which case putting the functionality in bitcoin-tx in the meantime seems plausible? Thoughts?

bitcoin-tx --grind feels weird to me. Grinding is unrelated to transactions. Unless we decide bitcoin-util is not desired, I think introducing it here makes more sense, personally.

"occassional runs" -> "occasional runs"

Done

Hi,

I usually run my binaries putting a symlink to them in a directory somewhere in my PATH (let's say /home/.local/bin, but it can be anything, really).

When doing this with miner, however, it won't work:

$ ln -s <base>/contrib/signet/miner /tmp/miner
$ /tmp/miner --help
Traceback (most recent call last):
  File "/tmp/miner", line 25, in <module>
    from test_framework.blocktools import WITNESS_COMMITMENT_HEADER, script_BIP34_coinbase_height # noqa: E402
ModuleNotFoundError: No module named 'test_framework'

The reason is that, when computing the absolute path to the directory containing miner, links are not followed. This would work if it included an invocation to realpath(), for example:

diff --git a/contrib/signet/miner b/contrib/signet/miner
--- a/contrib/signet/miner
+++ b/contrib/signet/miner
@@ -18,7 +18,7 @@ import subprocess
 from binascii import unhexlify
 from io import BytesIO
 
-PATH_BASE_CONTRIB_SIGNET = os.path.abspath(os.path.dirname(__file__))
+PATH_BASE_CONTRIB_SIGNET = os.path.abspath(os.path.dirname(os.path.realpath(__file__)))
 PATH_BASE_TEST_FUNCTIONAL = os.path.join(PATH_BASE_CONTRIB_SIGNET, "..", "..", "test", "functional")
 sys.path.insert(0, PATH_BASE_TEST_FUNCTIONAL)

On my copy, I also called abspath() when computing the final value for PATH_BASE_TEST_FUNCTIONAL, but that's just a question of taste, because I prefer <base>/test/functional instead of <base>/contrib/signet/../../test/functional in my include path (it's more explicit to debug if something goes wrong).

The final version would be:

diff --git a/contrib/signet/miner b/contrib/signet/miner
--- a/contrib/signet/miner
+++ b/contrib/signet/miner
@@ -18,8 +18,8 @@ import subprocess
 from binascii import unhexlify
 from io import BytesIO
 
-PATH_BASE_CONTRIB_SIGNET = os.path.abspath(os.path.dirname(__file__))
-PATH_BASE_TEST_FUNCTIONAL = os.path.join(PATH_BASE_CONTRIB_SIGNET, "..", "..", "test", "functional")
+PATH_BASE_CONTRIB_SIGNET = os.path.abspath(os.path.dirname(os.path.realpath(__file__)))
+PATH_BASE_TEST_FUNCTIONAL = os.path.abspath(os.path.join(PATH_BASE_CONTRIB_SIGNET, "..", "..", "test", "functional"))
 sys.path.insert(0, PATH_BASE_TEST_FUNCTIONAL)
 
 from test_framework.blocktools import WITNESS_COMMITMENT_HEADER, script_BIP34_coinbase_height # noqa: E402

From my experience with other Python projects, there should not be any undesired side effects, but I am not the expert here. Do you think supporting this use case makes sense?

Thanks!

Done

If miner calibrate is called without any additional arguments, there is a stack trace:

$ ./miner calibrate 
Traceback (most recent call last):
  File "./miner", line 637, in <module>
    main()
  File "./miner", line 631, in main
    return args.fn(args)
  File "./miner", line 541, in do_calibrate
    cmd = args.grind_cmd.split(" ") + [headhex]
AttributeError: 'NoneType' object has no attribute 'split'

One possible way to fix it could be making --grind-cmd required for the calibrate command. For --solvepsbt and --generate, apparently, this is not needed, since finish_block() checks for None already.

diff --git a/contrib/signet/miner b/contrib/signet/miner
--- a/contrib/signet/miner
+++ b/contrib/signet/miner
@@ -604,8 +604,8 @@ def main():
         sp.add_argument("--address", default=None, type=str, help="Address for block reward payment")
         sp.add_argument("--descriptor", default=None, type=str, help="Descriptor for block reward payment")
 
-    for sp in [solvepsbt, generate, calibrate]:
-        sp.add_argument("--grind-cmd", default=None, type=str, help="Command to grind a block header for proof-of-work")
+    for (sp, is_required) in [(solvepsbt, False), (generate, False), (calibrate, True)]:
+        sp.add_argument("--grind-cmd", default=None, type=str, required=is_required, help="Command to grind a block header for proof-of-work")
 
     args = parser.parse_args(sys.argv[1:])

Behaviour after the proposed change:

$ ./miner calibrate 
usage: miner calibrate [-h] [--nbits NBITS] [--seconds SECONDS] --grind-cmd GRIND_CMD
miner calibrate: error: the following arguments are required: --grind-cmd

A trailing '\n' at the end of this line would make is consistent with the previous sys.stderr.write():

diff --git a/contrib/signet/miner b/contrib/signet/miner
--- a/contrib/signet/miner
+++ b/contrib/signet/miner
@@ -520,7 +520,7 @@ def do_calibrate(args):
         sys.stderr.write("Can only specify one of --nbits or --seconds\n")
         return 1
     if args.nbits is not None and len(args.nbits) != 8:
-        sys.stderr.write("Must specify 8 hex digits for --nbits")
+        sys.stderr.write("Must specify 8 hex digits for --nbits\n")
         return 1
 
     TRIALS = 600 # gets variance down pretty low

typo here, should be ../contrib!

maybe just make this a one-liner with some ; or && 's?

How can i set this fixed interval? Slightly confused the difference between nbits and the interval itself. It sounds like interval is every 5 minutes try to mine a 25 seconds difficult block? What if I just want "blocks every 30 seconds"?

$GENERATE --cli="$CLI" "$@" generate --grind-cmd="$GRIND" --address="$ADDR" --nbits=$NBITS --multiminer=$MULTIMINER --ongoing --poisson

is what it looks like on my end. I think you want --ongoing --poisson.

I don't think --poisson is the right thing, as --poisson just adds noise?

I guess the parameter is standby delay, which defaults to 0?

Where I think I'm confused is how setting nbits works because we use the next_block_delta to define how long to wait before mining (via sleeping), but then the grind command conceivably takes est_time(nbits) to mine? so it feels like we're double delaying based on nbits?

You can't choose the interval; the difficulty will just get adjusted so the interval levels out at around 10 minutes. What you can target is a difficulty, so that once you reach that difficulty you keep mining a block every 10 minutes so that you stay at that difficulty. If you're not already at that difficulty, you instead choose a shorter/longer interval, so the difficulty is increased/decreased at the next retarget.

If you want to mine a bunch of blocks to get the chain started, then backdating via --set-block-time works. You could also use --nbits=170f1372 (current mainnet nbits) which would make it try to raise the difficulty by mining quickly (though not faster than an average of 2m30).

There's no double delay, because everything operates on the block time field -- ie, last block had timestamp T, next block has timestamp T+INTERVAL. The standby-delay and backup-delay parameters mean you won't start mining the block until T+INTERVAL+DELAY, and you won't finish mining the block until T+INTERVAL+DELAY+GRINDTIME, but those delays don't affect the following block, which will have a timestamp of T+INTERVAL+INTERVAL2 (though again you might not start mining it for a further DELAY, and it won't be finished until GRINDTIME has also passed)

Ahhh this makes sense I didn't realize normal difficulty adjustment rules are still used.

This text could go great in the Readme btw :)

Maybe incl a shell script template to backdate 100 blocks on start to seed a usable wallet?

I think just START=$(date +%s -d"1000 minutes ago") and --set-block-time=$START would work for that -- even with poisson timing you should end up mining 100+ blocks before you catch up with real time. Maybe something like that should just be the default if you're mining the first block of a signet chain?

mininmum > minimum

nbits=1e00f403 > --nbits=1e00f403

That line is an example of output of the command

The formatting looks strange when reading the README.md

Ah, apparently 4 spaces is the standard, not wordpress's 2 spaces. Fixed.

Maybe fix the formatting here also

Formatting here also

MINER="..contrib/signet/miner" > MINER="../contrib/signet/miner"

What happens when signet rules are set when calling getblocktemplate on a non-signet network, I suppose this is (or should be) an error?

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.

Thanks, clear.

I think this is awful. Can someone of the build system people please weigh in here?

This works for me:

diff --git a/src/Makefile.am b/src/Makefile.am
index b974bed66..f37db7bde 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -677,9 +677,9 @@ bitcoin_util_SOURCES += bitcoin-util-res.rc
 endif
 
 bitcoin_util_LDADD = \
-  $(LIBUNIVALUE) \
   $(LIBBITCOIN_COMMON) \
   $(LIBBITCOIN_UTIL) \
+  $(LIBUNIVALUE) \
   $(LIBBITCOIN_CONSENSUS) \
   $(LIBBITCOIN_CRYPTO) \
   $(LIBSECP256K1)
diff --git a/src/bitcoin-util.cpp b/src/bitcoin-util.cpp
index ff6bedec0..f7e670f4e 100644
--- a/src/bitcoin-util.cpp
+++ b/src/bitcoin-util.cpp
@@ -179,16 +179,6 @@ static int CommandLineUtil(int argc, char* argv[])
 
 int main(int argc, char* argv[])
 {
-    if (argc < 0) {
-        /* this nonsense is needed or you get linker errors because
-         * ArgsManager does these ops but for some reason they're not
-         * available.
-         * "argc < 0" should be enough to be unreachable, but not
-         * so unreachable that it's optimised away entirely
-         */
-        try { UniValue uv; uv["txid"]; uv.get_str(); uv.write(4); } catch (...) { }
-    }
-
     SetupEnvironment();
 
     try {

Thanks @fanquake !

Not sure if this include is required.