rpc: add gettarget , target getmininginfo field and show next block info #31583

pull Sjors wants to merge 15 commits into bitcoin:master from Sjors:2024/12/gettarget changing 26 files +4459 −40
  1. Sjors commented at 8:38 pm on December 30, 2024: member

    tl&dr for consensus-code only reviewers: the first commit splits CheckProofOfWorkImpl() in order to create a DeriveTarget() helper. The rest of this PR does not touch consensus code.

    There are three ways to represent the proof-of-work in a block:

    1. nBits
    2. Difficulty
    3. Target

    The latter notation is useful when you want to compare share work against either the pool target (to get paid) or network difficulty (found an actual block). E.g. for difficulty 1 which corresponds to an nBits value of 0x00ffff:

    0share hash: f6b973257df982284715b0c7a20640dad709d22b0b1a58f2f88d35886ea5ac45
1target:     7fffff0000000000000000000000000000000000000000000000000000000000

    It’s immediately clear that the share is invalid because the hash is above the target.

    This type of logging is mostly done by the pool software. It’s a nice extra convenience, but not very important. It impacts the following RPC calls:

    1. gettarget gives the target for the tip block, akin to getdifficulty
    2. getmininginfo displays the target for the tip block
    3. getblock and getblockheader display the target for a specific block (ditto for their REST equivalents)

    The gettarget and getdifficulty methods are a bit useless in their current state, because what miners really want to know if the target / difficulty for the next block. So I added a boolean argument next to getdifficulty and gettarget. (These values are typically the same, except for the first block in a retarget period. On testnet3 / testnet4 they change when no block is found after 20 minutes).

    Similarly I added a next object to getmininginfo which shows bit, difficulty and target for the next block.

    In order to test the difficulty transition, an alternate mainnet chain with 2016 blocks was generated and used in mining_mainnet.py. The chain is deterministic except for its timestamp and nonce values, which are stored in mainnet_alt.json.

    As described at the top, this PR introduces a helper method DeriveTarget() which is split out from CheckProofOfWorkImpl. The proposed checkblock RPC in #31564 needs this helper method internally to figure out the consensus target.

    Finally, this PR moves pow.cpp and chain.cpp from bitcoin_node to bitcoin_common, in order to give rpc/util.cpp (which lives in bitcoin_common) access to pow.h.

  2. DrahtBot commented at 8:38 pm on December 30, 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/31583.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK ryanofsky, tdb3
    Concept ACK ismaelsadeeq

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31664 (Fees: add Fee rate Forecaster Manager by ismaelsadeeq)
    • #31384 (mining: bugfix: Fix duplicate coinbase tx weight reservation by ismaelsadeeq)
    • #31375 (multiprocess: Add bitcoin wrapper executable by ryanofsky)
    • #31179 (RPC: Add reserve member function to UniValue and use it in blockToJSON function by ismaelsadeeq)
    • #30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
    • #27837 (net: introduce block tracker to retry to download blocks after failure by furszy)

    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.

  3. DrahtBot renamed this:
    rpc: add target to getmininginfo and introduce gettarget
    rpc: add target to getmininginfo and introduce gettarget
    on Dec 30, 2024
  4. DrahtBot added the label RPC/REST/ZMQ on Dec 30, 2024
  5. in test/functional/rpc_blockchain.py:446 in 5bba8625b0 outdated 
    438@@ -438,6 +439,12 @@ def _test_getdifficulty(self):
439         # binary => decimal => binary math is why we do this check
440         assert abs(difficulty * 2**31 - 1) < 0.0001
441 
442+    def _test_gettarget(self):
443+        self.log.info("Test gettarget")
444+        target = self.nodes[0].gettarget()
445+        # Target corresponding to nBits 0xffff7f20
446+        assert_equal(target, "7fffff0000000000000000000000000000000000000000000000000000000000")
    tdb3 commented at 0:52 am on December 31, 2024:
    nit: Since 7fffff0... is used both here and in mining_basic.py, might be worth de-duplicating? Maybe into test_framework/util.py?
    Sjors commented at 9:48 am on December 31, 2024:
    Done and also added test coverage for nBits.
  6. tdb3 commented at 1:06 am on December 31, 2024: contributor

    Approach ACK

    Welcome addition

    Sanity checked on mainnet (at block 200,000)

     0$ build/src/bitcoin-cli gettarget
 100000000000005db8b0000000000000000000000000000000000000000000000
 2$ build/src/bitcoin-cli getmininginfo
 3{
 4  "blocks": 200000,
 5  "difficulty": 2864140.507810974,
 6  "target": "00000000000005db8b0000000000000000000000000000000000000000000000",
 7  "networkhashps": 26411459642253.2,
 8  "pooledtx": 0,
 9  "chain": "main",
10  "warnings": [
11    "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
12  ]
13}
  7. Sjors force-pushed on Dec 31, 2024
  8. Sjors force-pushed on Dec 31, 2024
  9. Sjors renamed this:
    rpc: add target to getmininginfo and introduce gettarget
    rpc: add gettarget , target getmininginfo field and show next block info
    on Dec 31, 2024
  10. Sjors commented at 9:53 am on December 31, 2024: member

    Rebased and split things across more commits.

    I also realized that getmininginfo, getdifficulty and gettarget always refer to the tip, rather than the upcoming block which is what miners actually care about. So added a next argument to getdifficulty and gettarget and a next object to getmininginfo. You’ll have to test this manually, because regtest doesn’t have difficulty adjustment.

    I might push a test that uses the testnet4 genesis block and the rule that difficulty resets to 1 after 20 minutes.

  11. Sjors force-pushed on Dec 31, 2024
  12. Sjors commented at 10:45 am on December 31, 2024: member

    Added a helper function NextEmptyBlockIndex, which also fixes the missing nBits value on mainnet and signet.

    You can also test on mainnet by rolling back to the first block of the current retarget period: bitcoin-cli invalidateblock 0000000000000000000139773a9f91a256b5472f619b4bd297d07836a6587ffe

  13. DrahtBot added the label CI failed on Dec 31, 2024
  14. DrahtBot removed the label CI failed on Dec 31, 2024
  15. in src/rpc/mining.cpp:437 in 87a5eab16c outdated 
    433+                        {
434+                            {RPCResult::Type::NUM, "height", "The next height"},
435+                            {RPCResult::Type::STR_HEX, "bits", "The next target nBits"},
436+                            {RPCResult::Type::NUM, "difficulty", "The next difficulty"},
437+                            {RPCResult::Type::STR_HEX, "target", "The next target"}
438+                        }},
    tdb3 commented at 1:29 pm on December 31, 2024:

    Let’s add release notes for this, getdifficulty, and gettarget, e.g. something like:

    0Updated RPCs
1---
2- `getmininginfo` now returns the current target in the `target` field, and a `next` object, which specifies the `height`, `nBits`, `difficulty`, and `target` for the next block.
3- `getdifficulty` can now return the difficulty for the next block (rather than the current tip) when calling with the boolean `next` argument set to true.
4
5New RPCs
6---
7- `gettarget` can be used to return the current target (for tip) or target for the next block (with the `next` argument)
  16. tdb3 commented at 1:30 pm on December 31, 2024: contributor

    Approach ACK

    Planning to test later today as time allows. Left a minor comment, but it could wait until after testing (to avoid churn).

  17. Sjors force-pushed on Dec 31, 2024
  18. Sjors commented at 2:28 pm on December 31, 2024: member

    Added a testnet4 test, which includes the first 2016 real blocks. This takes up 1 MB (IIUC git effectively gzips that into 250kb).

    Also added release notes.

    I moved the NextEmptyBlockIndex helper from node/miner.h to rpc/util.h.

  19. Sjors commented at 2:49 pm on December 31, 2024: member

    I moved the NextEmptyBlockIndex helper from node/miner.h to rpc/util.h.

    That didn’t work, because rpc/util.cpp lives in libbitcoin_common, while the UpdateTime function it calls lives in libbitcoin_node. I moved it to rpc/server_util.{h,cpp} instead.

  20. Sjors force-pushed on Dec 31, 2024
  21. DrahtBot added the label CI failed on Dec 31, 2024
  22. DrahtBot commented at 2:50 pm on December 31, 2024: contributor

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

    Try to run the tests locally, according to the documentation. However, a CI failure may still 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.

  23. DrahtBot removed the label CI failed on Dec 31, 2024
  24. in test/functional/rpc_blockchain.py:453 in 7f822a8571 outdated 
    442@@ -443,11 +443,13 @@ def _test_getdifficulty(self):
443         # 1 hash in 2 should be valid, so difficulty should be 1/2**31
444         # binary => decimal => binary math is why we do this check
445         assert abs(difficulty * 2**31 - 1) < 0.0001
446+        assert_equal(self.nodes[0].getdifficulty(next=True), difficulty)
    tdb3 commented at 9:45 pm on December 31, 2024:

    Thinking out loud. nitty nit (feel free to ignore): Might not be obvious to the reader (the next block difficulty is the same as the current because there is no adjustment). If this file ends up being changed, could add a log message (or a comment), that is explicit.

    e.g.

    0self.log.info("Next difficulty should be the same as the current (no difficulty adjustment)")
1assert_equal(self.nodes[0].getdifficulty(next=True), difficulty)
    Sjors commented at 8:04 am on January 1, 2025:
    Will do if I need to retouch.
  25. in test/functional/mining_testnet.py:40 in 7f34802fd7 outdated 
    26+class MiningTestnetTest(BitcoinTestFramework):
27+
28+    def set_test_params(self):
29+        self.num_nodes = 1
30+        self.setup_clean_chain = True
31+        self.chain = "testnet4"
    tdb3 commented at 2:42 am on January 1, 2025:
    In the future, when testnet4 replaces testnet3 as -testnet (no trailing number), we’ll need to drop the 4.
    Sjors commented at 8:03 am on January 1, 2025:
    Maybe, depends on how soon testnet5 shows up :-)
  26. in test/functional/mining_testnet.py:45 in 7f34802fd7 outdated 
    31+        self.chain = "testnet4"
32+
33+    def add_options(self, parser):
34+        parser.add_argument(
35+            '--datafile',
36+            default='data/testnet4.hex',
    tdb3 commented at 2:42 am on January 1, 2025:
    Similarly here (file rename in the future)
    Sjors commented at 8:04 am on January 1, 2025:
    Note that this pattern is copied from a similar testnet3 test.
    ryanofsky commented at 10:54 pm on January 3, 2025:

    In commit “test: check testnet4 difficulty adjustment” (6b504d6cf8d0b3cf14b0df1683d080f505588e6b)

    Can there be some comment about how the testnet.4 file is generated and what it contains? Maybe there could be a readme in the test/functional/data/ directory.

    It seems ok to have one test like this with a large static data file, but it would be nice in the future to be able to generate block data more dynamically, and having a description of the data could help implement that.

    Sjors commented at 1:09 pm on January 4, 2025:

    Just this embarrassing bash script:

    0for i in {0..2015}
1do                                 
2hash=`build/src/bitcoin-cli -testnet4 getblockhash $i`
3block=`build/src/bitcoin-cli -testnet4 getblock $hash 0`
4echo $block >> test/functional/data/testnet4.hex
5done
  27. tdb3 commented at 2:44 am on January 1, 2025: contributor

    Seems like there is some history here regarding regtest and fPowNoRetargeting=true (https://github.com/bitcoin/bitcoin/pull/6853). When configured to avoid internet connectivity, it seems reasonable to me to use testnet4 and canned data (data/testnet4.hex).

    The bitcoin.conf for the test node has dnsseed=0, fixedseeds=0, and connect=0, which should help avoid internet traffic. Added a sleep in run_test() and didn’t see internet traffic from the test node in Wireshark.

    Planning to circle back and test a bit on mainnet.

  28. tdb3 approved
  29. tdb3 commented at 4:28 pm on January 1, 2025: contributor

    ACK 7f34802fd726b259f7df6f605e1d488faf251127

    Expected: same diff/target in next block

     0$ build/src/bitcoin-cli getblockhash 876959
 1000000000000000000002428ab473fa8570115d4a000e637814b86248d35d370
 2$ build/src/bitcoin-cli invalidateblock 000000000000000000002428ab473fa8570115d4a000e637814b86248d35d370
 3$ build/src/bitcoin-cli getdifficulty
 4108522647629298.2
 5$ build/src/bitcoin-cli getdifficulty true
 6108522647629298.2
 7$ build/src/bitcoin-cli gettarget
 80000000000000000000297fa0000000000000000000000000000000000000000
 9$ build/src/bitcoin-cli gettarget true
100000000000000000000297fa0000000000000000000000000000000000000000
11$ build/src/bitcoin-cli getmininginfo
12{
13  "blocks": 876958,
14  "bits": "170297fa",
15  "difficulty": 108522647629298.2,
16  "target": "0000000000000000000297fa0000000000000000000000000000000000000000",
17  "networkhashps": 8.339496073211752e+20,
18  "pooledtx": 3795,
19  "chain": "main",
20  "next": {
21    "height": 876959,
22    "bits": "170297fa",
23    "difficulty": 108522647629298.2,
24    "target": "0000000000000000000297fa0000000000000000000000000000000000000000"
25  },
26  "warnings": [
27    "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
28  ]
29}
30$ build/src/bitcoin-cli reconsiderblock 000000000000000000002428ab473fa8570115d4a000e637814b86248d35d370

    Expected: new diff/target in next block

     0$ build/src/bitcoin-cli getblockhash 876960
 10000000000000000000139773a9f91a256b5472f619b4bd297d07836a6587ffe
 2$ build/src/bitcoin-cli invalidateblock 0000000000000000000139773a9f91a256b5472f619b4bd297d07836a6587ffe
 3$ build/src/bitcoin-cli getdifficulty
 4108522647629298.2
 5$ build/src/bitcoin-cli getdifficulty true
 6109782075598905.2
 7$ build/src/bitcoin-cli gettarget
 80000000000000000000297fa0000000000000000000000000000000000000000
 9$ build/src/bitcoin-cli gettarget true
1000000000000000000002905c0000000000000000000000000000000000000000
11$ build/src/bitcoin-cli getmininginfo
12{
13  "blocks": 876959,
14  "bits": "170297fa",
15  "difficulty": 108522647629298.2,
16  "target": "0000000000000000000297fa0000000000000000000000000000000000000000",
17  "networkhashps": 8.214207064313692e+20,
18  "pooledtx": 3914,
19  "chain": "main",
20  "next": {
21    "height": 876960,
22    "bits": "1702905c",
23    "difficulty": 109782075598905.2,
24    "target": "00000000000000000002905c0000000000000000000000000000000000000000"
25  },
26  "warnings": [
27    "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
28  ]
29}
30$ build/src/bitcoin-cli reconsiderblock 0000000000000000000139773a9f91a256b5472f619b4bd297d07836a6587ffe

    Expected: same diff/target in next block

     0$ build/src/bitcoin-cli getblockhash 876961
 1000000000000000000024565da14cc4336d8713bcadbc889cb22ed73b940e048
 2$ build/src/bitcoin-cli invalidateblock 000000000000000000024565da14cc4336d8713bcadbc889cb22ed73b940e048
 3$ build/src/bitcoin-cli getdifficulty
 4109782075598905.2
 5$ build/src/bitcoin-cli getdifficulty true
 6109782075598905.2
 7$ build/src/bitcoin-cli gettarget
 800000000000000000002905c0000000000000000000000000000000000000000
 9$ build/src/bitcoin-cli gettarget true
1000000000000000000002905c0000000000000000000000000000000000000000
11$ build/src/bitcoin-cli getmininginfo
12{
13  "blocks": 876960,
14  "bits": "1702905c",
15  "difficulty": 109782075598905.2,
16  "target": "00000000000000000002905c0000000000000000000000000000000000000000",
17  "networkhashps": 8.133301750223401e+20,
18  "pooledtx": 4022,
19  "chain": "main",
20  "next": {
21    "height": 876961,
22    "bits": "1702905c",
23    "difficulty": 109782075598905.2,
24    "target": "00000000000000000002905c0000000000000000000000000000000000000000"
25  },
26  "warnings": [
27    "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
28  ]
29}
30$ build/src/bitcoin-cli reconsiderblock 000000000000000000024565da14cc4336d8713bcadbc889cb22ed73b940e048
  30. tdb3 commented at 0:53 am on January 2, 2025: contributor
    Looks like we can re-use the new DIFF_1_N_BITS and REGTEST_N_BITS in a couple other tests (https://github.com/tdb3/bitcoin/commits/2024/12/gettarget/). Could be added if there are other changes or left for future. The use of REGTEST_N_BITS might be borderline (the test is creating blocks by hand).
  31. Sjors commented at 9:21 am on January 2, 2025: member
    @tdb3 thanks, I’ll cherry-pick those two commits if any other changes are needed here.
  32. in src/pow.cpp:14 in 2f549b5472 outdated 
    10@@ -11,6 +11,20 @@
11 #include <uint256.h>
12 #include <util/check.h>
13 
14+bool DeriveTarget(unsigned int nBits, const uint256 pow_limit, arith_uint256& target)
    ryanofsky commented at 9:32 pm on January 2, 2025:

    In commit “Add DeriveTarget() to pow.h” (2f549b547207839ab666ac2b8b02811ff0990214)

    I don’t really like the approach here of duplicating the CheckProofOfWorkImpl code and having a comment saying a future refactor could deduplicate it. I think it could avoid confusion and bugs later on to just split the CheckProofOfWorkImpl function into two parts now in a way that makes it obvious behavior is not changing. Would suggest:

     0--- a/src/pow.cpp
 1+++ b/src/pow.cpp
 2@@ -143,7 +143,7 @@ bool CheckProofOfWork(uint256 hash, unsigned int nBits, const Consensus::Params&
 3     return CheckProofOfWorkImpl(hash, nBits, params);
 4 }
 5 
 6-bool CheckProofOfWorkImpl(uint256 hash, unsigned int nBits, const Consensus::Params& params)
 7+std::optional<arith_uint256> DeriveTarget(unsigned int nBits, const uint256 pow_limit)
 8 {
 9     bool fNegative;
10     bool fOverflow;
11@@ -152,8 +152,16 @@ bool CheckProofOfWorkImpl(uint256 hash, unsigned int nBits, const Consensus::Par
12     bnTarget.SetCompact(nBits, &fNegative, &fOverflow);
13 
14     // Check range
15-    if (fNegative || bnTarget == 0 || fOverflow || bnTarget > UintToArith256(params.powLimit))
16-        return false;
17+    if (fNegative || bnTarget == 0 || fOverflow || bnTarget > UintToArith256(pow_limit))
18+        return {};
19+
20+    return bnTarget;
21+}
22+
23+bool CheckProofOfWorkImpl(uint256 hash, unsigned int nBits, const Consensus::Params& params)
24+{
25+    auto bnTarget{DeriveTarget(nBits, params.powLimit)};
26+    if (!bnTarget) return false;
27 
28     // Check proof of work matches claimed amount
29     if (UintToArith256(hash) > bnTarget)
30--- a/src/pow.h
31+++ b/src/pow.h
32@@ -13,6 +13,18 @@
33 class CBlockHeader;
34 class CBlockIndex;
35 class uint256;
36+class arith_uint256;
37+
38+/**
39+ * Convert nBits value to target.
40+ *
41+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] nBits     compact representation of the target
42+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] pow_limit PoW limit (consensus parameter)
43+ *
44+ * [@return](/bitcoin-bitcoin/contributor/return/)              the proof-of-work target or nullopt if the nBits value
45+ *                      is invalid (due to overflow or exceeding pow_limit)
46+ */
47+std::optional<arith_uint256> DeriveTarget(unsigned int nBits, const uint256 pow_limit);
48 
49 unsigned int GetNextWorkRequired(const CBlockIndex* pindexLast, const CBlockHeader *pblock, const Consensus::Params&);
50 unsigned int CalculateNextWorkRequired(const CBlockIndex* pindexLast, int64_t nFirstBlockTime, const Consensus::Params&);

    I also think would be less-error prone if DeriveTarget returns std::optional<arith_uint256> instead of a bool and separate arith_uint256& output parameter.

    Sjors commented at 10:00 am on January 3, 2025:
    Done in 6accee18442f79fd2f2796027371a70e3757d825.
  33. in src/rpc/blockchain.cpp:556 in ecb18b3405 outdated 
    552@@ -553,7 +553,7 @@ static RPCHelpMan getblockheader()
553                             {RPCResult::Type::NUM_TIME, "time", "The block time expressed in " + UNIX_EPOCH_TIME},
554                             {RPCResult::Type::NUM_TIME, "mediantime", "The median block time expressed in " + UNIX_EPOCH_TIME},
555                             {RPCResult::Type::NUM, "nonce", "The nonce"},
556-                            {RPCResult::Type::STR_HEX, "bits", "The bits"},
557+                            {RPCResult::Type::STR_HEX, "bits", "The target nBits"},
    ryanofsky commented at 10:41 pm on January 2, 2025:

    In commit “test: add nBits coverage” (ecb18b3405e4a61f872d03f667c167fb7c4a52cf)

    I think it would be good to make this more descriptive. Maybe add “Compact representation of the block difficulty target, which the block hash interpreted as a little-endian number must not be greater than. This is hexadecimal string containing 32-bit number with the compact representation described in the arith_uint256 class of the difficulty target.”

    I also think it’s really unfortunate the RPC is returning this compact representation, and it would be better if RPCs returned uncompressed difficulty targets which would be easier to understand and are directly comparable to block hashes. e.g.

    0result.pushKV("hash", blockindex.GetBlockHash().GetHex());
1arith_uint256 target;
2target.SetCompact(blockindex.nBits);
3result.pushKV("target", ArithToUint256(target).GetHex());

    EDIT: I see later commits actually do add “target” fields which do this. In that case, I think it would be good to just document bits fields as a containing a compact representation of the target fields, since the target representation is a lot more straightforward.

    Sjors commented at 10:00 am on January 3, 2025:

    Ok, I used “nBits: compact representation of the block difficulty target”.

    To avoid confusion, I introduced a new commit e4f02e5aca367591cd37f9f8c46776b0f7e3ce2b that adds a “target” field to getblockheader and getblock (as well as their REST equivalents).

  34. in test/functional/test_framework/blocktools.py:77 in 0c56689e42 outdated 
    72 
73 def nbits_str(nbits):
74     return nbits.to_bytes(4, "big").hex()
75 
76+def target_str(target):
77+    return ser_uint256(target)[::-1].hex()
    ryanofsky commented at 10:54 pm on January 2, 2025:

    In commit “rpc: gettarget” (0c56689e425684c0c5b2058c1a47f293ac7032e6)

    It’s not right to involve the ser_uint256 function here, since converting the target int to a string should not involve bitcoin serialization. This can just use python string formatting return f"{target:064x}"

    Sjors commented at 9:59 am on January 3, 2025:
    Taken
  35. in test/functional/test_framework/blocktools.py:71 in ecb18b3405 outdated 
    64@@ -65,6 +65,10 @@
65 VERSIONBITS_LAST_OLD_BLOCK_VERSION = 4
66 MIN_BLOCKS_TO_KEEP = 288
67 
68+REGTEST_N_BITS = 0x207fffff  # difficulty retargeting is disabled in REGTEST chainparams"
69+
70+def nbits_str(nbits):
71+    return nbits.to_bytes(4, "big").hex()
    ryanofsky commented at 11:01 pm on January 2, 2025:

    In commit “test: add nBits coverage” (ecb18b3405e4a61f872d03f667c167fb7c4a52cf)

    return f"{nbits:08x}" would be a more direct conversion from number to string and would make the python code more consistent with the c++ code.

    Sjors commented at 9:59 am on January 3, 2025:
    Taken
  36. ryanofsky commented at 11:17 pm on January 2, 2025: contributor

    Code review 7f34802fd726b259f7df6f605e1d488faf251127. I got pretty lost here and only reviewed about half of it so far, but overall I think the changes look good and make a lot of sense.

    I left some suggestions below which I think could make code clearer, but they are not important so feel free to ignore any you disagree with.

  37. ryanofsky referenced this in commit 13464c0c44 on Jan 2, 2025
  38. Sjors force-pushed on Jan 3, 2025
  39. Sjors force-pushed on Jan 3, 2025
  40. Sjors commented at 10:00 am on January 3, 2025: member

    Thanks for the review @ryanofsky. I took your suggestion to split CheckProofOfWorkImpl. Can you add the Consensus label to this PR? @tdb3 I took your two patches and one suggestion

    Edited the PR description to reflect these changes.

  41. Sjors force-pushed on Jan 3, 2025
  42. DrahtBot added the label CI failed on Jan 3, 2025
  43. DrahtBot commented at 10:15 am on January 3, 2025: contributor

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

    Try to run the tests locally, according to the documentation. However, a CI failure may still 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.

  44. DrahtBot removed the label CI failed on Jan 3, 2025
  45. ryanofsky referenced this in commit 3e0a992a3f on Jan 3, 2025
  46. tdb3 commented at 7:49 pm on January 3, 2025: contributor

    Approach ACK

    Reviewed code changes.

    Syncing from genesis on mainnet (because we’re touching CheckProofOfWorkImpl(), I don’t mind being paranoid, and it happens to be the genesis block anniversary today), so will report back (with ACK if all is well).

    Looks like we could add target to getblockchaininfo and getchainstates as well (the top two commits on https://github.com/tdb3/bitcoin/commits/2024/12/gettarget/ add target and associated tests). Could be left for follow up if desired.

  47. ryanofsky added the label Consensus on Jan 3, 2025
  48. ryanofsky approved
  49. ryanofsky commented at 10:58 pm on January 3, 2025: contributor
    Code review ACK b35ed41b03578586e380cb73aece14046ec2da93. Nice changes that make the mining interface more useful and complete, and are cleanly implemented
  50. DrahtBot requested review from tdb3 on Jan 3, 2025
  51. tdb3 approved
  52. tdb3 commented at 6:01 am on January 4, 2025: contributor

    Code review re ACK b35ed41b03578586e380cb73aece14046ec2da93

    Sync from genesis was successful (to 877742, 000000000000000000008bac930b7efd54a7ddef0b9ec5b1c0c00b6a6c8e0cfb)

    fyi, conf used:

    0[main]
1connect=<sister node on LAN>
2checkpoints=0
3prune=53000
4dbcache=24000
  53. Sjors force-pushed on Jan 4, 2025
  54. Sjors commented at 1:43 pm on January 4, 2025: member

    Changes:

    • dropped testnet4 genesis block
    • explained in the test how to generate / verify testnet4.hex
    • added bits and target to getblockchaininfo and getchainstates (didn’t realise @tdb3 had already implemented it)
  55. in src/rpc/blockchain.cpp:1409 in 96a5bfd770 outdated 
    1404     obj.pushKV("headers", chainman.m_best_header ? chainman.m_best_header->nHeight : -1);
1405     obj.pushKV("bestblockhash", tip.GetBlockHash().GetHex());
1406+    obj.pushKV("bits", strprintf("%08x", tip.nBits));
1407+    obj.pushKV("target", GetTarget(tip, pow_limit).GetHex());
1408     obj.pushKV("difficulty", GetDifficulty(tip));
1409+    obj.pushKV("target", GetTarget(tip, pow_limit).GetHex());
    tdb3 commented at 2:10 pm on January 4, 2025:
    Looks like this line is a duplicate (target added twice)
    Sjors commented at 9:01 am on January 6, 2025:
    Fixed
  56. tdb3 commented at 2:15 pm on January 4, 2025: contributor

    Approach ACK

    Verified the addition of bits and target to getblockchaininfo and getchainstates on mainnet:

     0$ build/src/bitcoin-cli getblockchaininfo
 1{
 2  "chain": "main",
 3  "blocks": 877759,
 4  "headers": 877796,
 5  "bestblockhash": "00000000000000000001cdf88323d957f92d07693b953c32646bac7c797634af",
 6  "bits": "1702905c",
 7  "target": "00000000000000000002905c0000000000000000000000000000000000000000",
 8  "difficulty": 109782075598905.2,
 9  "time": 1735981113,
10  "mediantime": 1735976878,
11  "verificationprogress": 0.9998884052581367,
12  "initialblockdownload": false,
13  "chainwork": "0000000000000000000000000000000000000000a50ce26166c7c567206d3760",
14  "size_on_disk": 55429575203,
15  "pruned": true,
16  "pruneheight": 848361,
17  "automatic_pruning": true,
18  "prune_target_size": 55574528000,
19  "warnings": [
20    "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
21  ]
22}
23$ build/src/bitcoin-cli getchainstates
24{
25  "headers": 877796,
26  "chainstates": [
27    {
28      "blocks": 877778,
29      "bestblockhash": "0000000000000000000036bc3aba12ed13cc759cba2a55309d3b3cd3d9183a8a",
30      "bits": "1702905c",
31      "target": "00000000000000000002905c0000000000000000000000000000000000000000",
32      "difficulty": 109782075598905.2,
33      "verificationprogress": 0.999934824273136,
34      "coins_db_cache_bytes": 8388608,
35      "coins_tip_cache_bytes": 24081596416,
36      "validated": true
37    }
38  ]
39}
  57. Sjors force-pushed on Jan 6, 2025
  58. achow101 referenced this in commit 433412fd84 on Jan 6, 2025
  59. ismaelsadeeq referenced this in commit 5b370d8483 on Jan 7, 2025
  60. tdb3 approved
  61. tdb3 commented at 0:46 am on January 8, 2025: contributor
    Code review re ACK 223e8bcb9990834a5c9ffcd8a79f617a6f2ecfc1
  62. DrahtBot requested review from ryanofsky on Jan 8, 2025
  63. in test/functional/mining_testnet.py:16 in 223e8bcb99 outdated 
    11+
12+for i in {1..2015}
13+do
14+  hash=`bitcoin-cli -testnet4 getblockhash $i`
15+  block=`bitcoin-cli -testnet4 getblock $hash 0`
16+  echo $block >> data/testnet4.hex
    maflcko commented at 8:02 am on January 8, 2025:
    Not sure about adding a file to the git repo whose worst case size would be 8GB (possibly for testnet5). Given that you only need the headers, it could make sense to mine a minimal chain and only commit the nonces. An alternative would be to use the main chain, which doesn’t have to be changed again in the future.
    Sjors commented at 10:49 am on January 8, 2025:

    I thought about generating a fake testnet4 with empty blocks, but that wouldn’t be much smaller. However if testnet5 turns out to have large initial blocks, we can always do that (or commit the nonces).

    Headers are not enough, because I need the tip to update. Unless you want to revive #9483 :-)

    Sjors commented at 10:50 am on January 8, 2025:

    That said, committing only 2016 nonces would make it a lot smaller, so maybe it’s a good idea regardless.

    I guess I could use faketime to keep the timestamps constant or exactly 10 minutes apart?

    I would probably use mainnet in that case.

  64. in src/rpc/server_util.cpp:140 in 223e8bcb99 outdated 
    132@@ -129,3 +133,18 @@ AddrMan& EnsureAnyAddrman(const std::any& context)
133 {
134     return EnsureAddrman(EnsureAnyNodeContext(context));
135 }
136+
137+void NextEmptyBlockIndex(CBlockIndex* tip, const Consensus::Params& consensusParams, CBlockIndex& next_index)
138+{
139+    CBlockHeader next_header{};
140+    next_header.hashPrevBlock  = tip->GetBlockHash();
    maflcko commented at 8:03 am on January 8, 2025:

    nit: The function does not accept nullptr, so it would be good to document it properly.

    0void NextEmptyBlockIndex(CBlockIndex& tip, const Consensus::Params& consensusParams, CBlockIndex& next_index)
1{
2    CBlockHeader next_header{};
3    next_header.hashPrevBlock  = tip.GetBlockHash();
    Sjors commented at 11:35 am on January 8, 2025:

    This results in assigning to 'CBlockIndex *' from 'const CBlockIndex *' discards qualifier at the line next_index.pprev = &tip;.

    I’d rather not enter the rabbit hole of changing CBlockIndex to make pprev as const CBlockIndex*. So for now I just drop the const qualifier for tip. Unless you have another suggestion?

    maflcko commented at 11:42 am on January 8, 2025:

    I’d rather not enter the rabbit hole of changing CBlockIndex to make pprev as const CBlockIndex*.

    It’s fine, my bad. It needs to be mutable, so I edited my nit suggestion.

  65. in src/rpc/server_util.cpp:7 in 223e8bcb99 outdated 
    3@@ -4,10 +4,13 @@
4 
5 #include <rpc/server_util.h>
6 
7+#include <chain.h> // for CBlockIndex
    maflcko commented at 8:04 am on January 8, 2025:
    nit: I don’t think the comment (and maintenance overhead) should be added.
  66. in src/rpc/blockchain.cpp:848 in 223e8bcb99 outdated 
    842@@ -781,6 +843,10 @@ static RPCHelpMan getblock()
843         if (!pblockindex) {
844             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
845         }
846+
847+        // Now that we have a lock on cs_main, get powLimit so blockToJSON()
848+        // doesn't have to.
    maflcko commented at 8:09 am on January 8, 2025:

    The comment is wrong, because cs_main isn’t needed to call a getter that returns a const reference to a const blob.

    So it would be better to move the call out of cs_main to avoid that impression, and to remove the comment.

    Also, instead of chainman.GetParams().GetConsensus(), you can simply call chainman.GetConsensus().

    (This comment applies to the whole diff)

    Sjors commented at 11:15 am on January 8, 2025:

    I switched to using chainman.GetConsensus() everywhere.

    The lock on cs_main is needed to get pblockindex and tip, but it’s not needed to get chainman and therefore also not needed to get pow_limit. I’ll move some things around.

  67. in src/rpc/blockchain.cpp:485 in 223e8bcb99 outdated 
    481+                },
482+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
483+{
484+    ChainstateManager& chainman = EnsureAnyChainman(request.context);
485+    LOCK(cs_main);
486+    CBlockIndex* tip{CHECK_NONFATAL(chainman.ActiveChain().Tip())};
    maflcko commented at 8:13 am on January 8, 2025:

    nit: Not that it matters for performance, but for documentation, it would be good to limit cs_main to the only call that needs it.

    0
1    const CBlockIndex& tip{*WITH_LOCK(chainman.GetMutex(), return CHECK_NONFATAL(chainman.ActiveChain().Tip()))};

    (same above, …)

    Sjors commented at 11:05 am on January 8, 2025:
    I had to move CHECK_NONFATAL outside the WITH_LOCK call to avoid returning reference to local temporary object [-Wreturn-stack-address]
  68. in doc/release-notes-31583.md:14 in 223e8bcb99 outdated 
     9+---
10+- `gettarget` can be used to return the current target (for tip) or target for the next block (with the `next` argument)
11+
12+REST interface
13+---
14+- `GET /rest/block/<BLOCK-HASH>` and `GET /rest/headers/<BLOCK-HASH>` now return the current target in the `target` field
    maflcko commented at 8:17 am on January 8, 2025:
    I don’t think this is right. The .json suffix is missing? (It is not possible to return a “field” in the other formats)
    Sjors commented at 10:41 am on January 8, 2025:

    For the web developer in me that’s obvious :-)

    But I’ll add .json to make it more clear this doesn’t apply to the other formats.

  69. Sjors force-pushed on Jan 8, 2025
  70. Sjors commented at 12:19 pm on January 8, 2025: member

    Addressed comments. Looking into replacing testnet4.hex with something much more compact.

    (update: I refactored the test and currently generating 2016 difficulty 1 blocks with realistic timestamps, will push when its ready)

  71. ryanofsky approved
  72. ryanofsky commented at 7:12 pm on January 8, 2025: contributor

    Code review ACK d925a9bd7a04f068ede227addbaef3025b9793b9. Lot of nice simplifications since last review: using references instead of pointers, getting rid of intermediate variables, using chainman.GetConsensus(), also improving release notes and test documentation and updating more RPCs to return the bits and target fields.

    It would be really nice to remove the large test data file, so happy to rereview when the new test is ready.

  73. DrahtBot requested review from tdb3 on Jan 8, 2025
  74. tdb3 approved
  75. tdb3 commented at 2:58 am on January 9, 2025: contributor

    Code review re ACK d925a9bd7a04f068ede227addbaef3025b9793b9

    Great updates. Would review again if an alternate test approach for data/testnet4.hex was used. I also like the idea of possibly using early mainnet (wouldn’t need to be changed). Weighing in at ~903KB (hex), using mainnet is smaller than the 1.2MB of testnet4 block data. A nonce approach could be even nicer/smaller.

    There are other areas where .GetParams().GetConsensus() could be replaced with .GetConsensus(), but I think it’s prudent to prevent touching other code unnecessarily and to prevent further scope creep on this PR.

     0diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
 1index 1070e8f68f..abc4b44b44 100644
 2--- a/src/rpc/blockchain.cpp
 3+++ b/src/rpc/blockchain.cpp
 4@@ -1244,7 +1244,7 @@ static RPCHelpMan verifychain()
 5 
 6     Chainstate& active_chainstate = chainman.ActiveChainstate();
 7     return CVerifyDB(chainman.GetNotifications()).VerifyDB(
 8-               active_chainstate, chainman.GetParams().GetConsensus(), active_chainstate.CoinsTip(), check_level, check_depth) == VerifyDBResult::SUCCESS;
 9+               active_chainstate, chainman.GetConsensus(), active_chainstate.CoinsTip(), check_level, check_depth) == VerifyDBResult::SUCCESS;
10 },
11     };
12 }
13@@ -1410,7 +1410,7 @@ RPCHelpMan getblockchaininfo()
14     }
15     if (chainman.GetParams().GetChainType() == ChainType::SIGNET) {
16         const std::vector<uint8_t>& signet_challenge =
17-            chainman.GetParams().GetConsensus().signet_challenge;
18+            chainman.GetConsensus().signet_challenge;
19         obj.pushKV("signet_challenge", HexStr(signet_challenge));
20     }
21 
22@@ -1781,7 +1781,7 @@ static RPCHelpMan getchaintxstats()
23 {
24     ChainstateManager& chainman = EnsureAnyChainman(request.context);
25     const CBlockIndex* pindex;
26-    int blockcount = 30 * 24 * 60 * 60 / chainman.GetParams().GetConsensus().nPowTargetSpacing; // By default: 1 month
27+    int blockcount = 30 * 24 * 60 * 60 / chainman.GetConsensus().nPowTargetSpacing; // By default: 1 month
28 
29     if (request.params[1].isNull()) {
30         LOCK(cs_main);
31@@ -2121,7 +2121,7 @@ static RPCHelpMan getblockstats()
32     ret_all.pushKV("minfeerate", (minfeerate == MAX_MONEY) ? 0 : minfeerate);
33     ret_all.pushKV("mintxsize", mintxsize == MAX_BLOCK_SERIALIZED_SIZE ? 0 : mintxsize);
34     ret_all.pushKV("outs", outputs);
35-    ret_all.pushKV("subsidy", GetBlockSubsidy(pindex.nHeight, chainman.GetParams().GetConsensus()));
36+    ret_all.pushKV("subsidy", GetBlockSubsidy(pindex.nHeight, chainman.GetConsensus()));
37     ret_all.pushKV("swtotal_size", swtotal_size);
38     ret_all.pushKV("swtotal_weight", swtotal_weight);
39     ret_all.pushKV("swtxs", swtxs);
40diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
41index e15c4e62c6..bb193f1ea1 100644
42--- a/src/rpc/mining.cpp
43+++ b/src/rpc/mining.cpp
44@@ -817,7 +817,7 @@ static RPCHelpMan getblocktemplate()
45         // TODO: Maybe recheck connections/IBD and (if something wrong) send an expires-immediately template to stop miners?
46     }
47 
48-    const Consensus::Params& consensusParams = chainman.GetParams().GetConsensus();
49+    const Consensus::Params& consensusParams = chainman.GetConsensus();
50 
51     // GBT must be called with 'signet' set in the rules for signet chains
52     if (consensusParams.signet_blocks && setClientRules.count("signet") != 1) {
53diff --git a/src/test/fuzz/p2p_headers_presync.cpp b/src/test/fuzz/p2p_headers_presync.cpp
54index 873eb2b1cc..7fb2381fa7 100644
55--- a/src/test/fuzz/p2p_headers_presync.cpp
56+++ b/src/test/fuzz/p2p_headers_presync.cpp
57@@ -135,7 +135,7 @@ CBlock ConsumeBlock(FuzzedDataProvider& fuzzed_data_provider, const uint256& pre
58 
59 void FinalizeHeader(CBlockHeader& header, const ChainstateManager& chainman)
60 {
61-    while (!CheckProofOfWork(header.GetHash(), header.nBits, chainman.GetParams().GetConsensus())) {
62+    while (!CheckProofOfWork(header.GetHash(), header.nBits, chainman.GetConsensus())) {
63         ++(header.nNonce);
64     }
65 }
  76. Sjors force-pushed on Jan 9, 2025
  77. Sjors commented at 9:48 am on January 9, 2025: member

    Here we go!

    I generated an alternate mainnet with 2016 blocks, storing timestamps and nonces. The process is explained at the top of the renamed test file: ee1dcc6074323ae361e28cf235ddfc9bf07be1da.

    I did leave out some minutia for readability sake, and because things will change over time. E.g. I had to disable the miner.isTestChain()) guard in getblocktemplate, I patched the CPU miner to work with taproot payout addresses https://github.com/pooler/cpuminer/pull/293 and set the coinbase version to https://github.com/pooler/cpuminer/pull/294. However it’s trivial to work around those when reconstructing or expanding these test vectors.

    I also left out some rpc and jq incantations to generate the json file.

    47bf1086dacfc504699d672dc3faf8a77eceee67 changes the BIP34 padding from OP_1 to OP_0 in the test suite, which will also impact new signets (but it shouldn’t matter).

  78. in test/functional/test_framework/blocktools.py:131 in 47bf1086da outdated 
    128@@ -129,7 +129,7 @@ def script_BIP34_coinbase_height(height):
129     if height <= 16:
130         res = CScriptOp.encode_op_n(height)
131         # Append dummy to increase scriptSig size above 2 (see bad-cb-length consensus rule)
    tdb3 commented at 5:00 pm on January 11, 2025:

    nit: Is it 2 and above?

    0- above 2
1+ to at least 2

    https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/src/consensus/tx_check.cpp#L47-L51

    Sjors commented at 8:58 am on January 13, 2025:
    I changed it to “to 2”
  79. in test/functional/tool_signet_miner.py:59 in 9c646bf4f5 outdated 
    55@@ -55,7 +56,7 @@ def run_test(self):
56                 'generate',
57                 f'--address={node.getnewaddress()}',
58                 f'--grind-cmd={self.options.bitcoinutil} grind',
59-                '--nbits=1d00ffff',
60+                f'--nbits={hex(DIFF_1_N_BITS)}',
    tdb3 commented at 5:53 pm on January 11, 2025:

    If retouching, looks like we can adjust

    0- f'--nbits={hex(DIFF_1_N_BITS)}',
1+ f'--nbits={DIFF_1_N_BITS:08x}',

    Currently the 0x is included. The test runs fine with it currently.

    Looks like parts of miner expect no 0x though (8 hex digits):

    https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/contrib/signet/miner#L465-L467

    https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/contrib/signet/miner#L356-L359

    0>>> f'--nbits={hex(DIFF_1_N_BITS)}'
1'--nbits=0x1d00ffff'
2>>> f'--nbits={DIFF_1_N_BITS:08x}'
3'--nbits=1d00ffff'
    Sjors commented at 8:58 am on January 13, 2025:
    Fixed
  80. in test/functional/mining_mainnet.py:35 in 58b828f7f7 outdated 
    32+having to redo the proof-of-work.
33+
34+The timestamp was not kept constant because at difficulty 1 it's not sufficient
35+to only grind the nonce. Grinding the extra_nonce or version field instead
36+would have required additional (stratum) software. It would also make it more
37+complicated to reconstruct the blocks in this test.
    tdb3 commented at 7:26 pm on January 11, 2025:
    Wonder if it would be sufficient to grind only the nonce if starting with a testnet genesis block (instead of mainnet), but not sure it’s worth digging into. It might allow removing the timestamps from mainnet_alt.json (reducing 35KB of the current 71KB), but the current approach is already much more manageable that the previous ~1MB.
    Sjors commented at 8:47 am on January 13, 2025:
    Testnet3, testnet4 and mainnet all have a difficulty 1 genesis block, so it wouldn’t make a difference.
  81. tdb3 approved
  82. tdb3 commented at 7:27 pm on January 11, 2025: contributor

    code review re ACK 58b828f7f7ad216c7d7bf2e2ff431a66591e9d5c

    The data file is down to 71K, which is much nicer!

    mining_mainnet.py ran locally without issue.

  83. DrahtBot requested review from ryanofsky on Jan 11, 2025
  84. Sjors force-pushed on Jan 13, 2025
  85. Sjors commented at 6:47 pm on January 13, 2025: member

    After discussion in #31625 I realized that the coinbase transactions here are unspendable due to the unexpected-witness and hardcode SegWit activation height on mainnet.

    It doesn’t matter for this PR, but I’m working on a new edition that uses legacy addresses and also checks that the coinbase is spendable. Will push here or, if it’s already merged, to a followup.

  86. tdb3 approved
  87. tdb3 commented at 0:58 am on January 14, 2025: contributor

    code review re ACK b945668216abbc978ce351b8a89910e651c6e595

    Changes were to address the latest comments (removing “0x” and adjusting a comment).

     0$ git range-diff 228aba2c4d9ac0b2ca3edd3c2cdf0a92e55f669b..58b828f7f7ad216c7d7bf2e2ff431a66591e9d5c e8ea483fec05d824d2f35182b1a3bd68416b2df3~..b945668216abbc978ce351b8a89910e651c6e595
 1 1:  6accee1844 =  1:  e8ea483fec consensus: add DeriveTarget() to pow.h
 2 2:  3e976d4171 =  2:  6a72497f50 build: move pow and chain to bitcoin_common
 3 3:  2401931542 =  3:  c370fa33c2 rpc: add nBits to getmininginfo
 4 4:  ddd2ede17a =  4:  c50ea52534 test: use REGTEST_N_BITS in feature_block
 5 5:  ef9e86039a =  5:  4db86d8638 rpc: gettarget
 6 6:  a0145ce476 =  6:  0c3b0af556 Add target to getblock(header) in RPC and REST
 7 7:  f06f55090e =  7:  684e3247d2 rpc: add target to getmininginfo result
 8 8:  1502b98b8b =  8:  333a146882 rpc: add next to getdifficulty and gettarget
 9 9:  590d3ab92c =  9:  b0f82495a9 rpc: add target and bits to getblockchaininfo
1010:  4463b0e04c = 10:  02ba73a4a0 rpc: add target and bits to getchainstates
1111:  a9ce7d1774 = 11:  e9f5b618dc rpc: add next to getmininginfo
1212:  47bf1086da ! 12:  047b471a87 Use OP_0 for BIP34 padding in signet and tests
13    @@ test/functional/test_framework/blocktools.py: from .script import (
14          OP_RETURN,
15          OP_TRUE,
16      )
17    -@@ test/functional/test_framework/blocktools.py: def script_BIP34_coinbase_height(height):
18    +@@ test/functional/test_framework/blocktools.py: def add_witness_commitment(block, nonce=0):
19    + def script_BIP34_coinbase_height(height):
20          if height <= 16:
21              res = CScriptOp.encode_op_n(height)
22    -         # Append dummy to increase scriptSig size above 2 (see bad-cb-length consensus rule)
23    +-        # Append dummy to increase scriptSig size above 2 (see bad-cb-length consensus rule)
24     -        return CScript([res, OP_1])
25    ++        # Append dummy to increase scriptSig size to 2 (see bad-cb-length consensus rule)
26     +        return CScript([res, OP_0])
27          return CScript([CScriptNum(height)])
28      
2913:  ee1dcc6074 = 13:  4138351f60 test: check difficulty adjustment using alternate mainnet
3014:  9c646bf4f5 ! 14:  289e304ac3 test: use DIFF_1_N_BITS in tool_signet_miner
31    @@ test/functional/tool_signet_miner.py: class SignetMinerTest(BitcoinTestFramework
32                      f'--address={node.getnewaddress()}',
33                      f'--grind-cmd={self.options.bitcoinutil} grind',
34     -                '--nbits=1d00ffff',
35    -+                f'--nbits={hex(DIFF_1_N_BITS)}',
36    ++                f'--nbits={DIFF_1_N_BITS:08x}',
37                      f'--set-block-time={int(time.time())}',
38                      '--poolnum=99',
39                  ], check=True, stderr=subprocess.STDOUT)
4015:  58b828f7f7 = 15:  b945668216 doc: add release notes

    Good catch about the unspendable coinbase outputs. I’d re-review the updated approach in either case (in this PR or a new one).

  88. Sjors force-pushed on Jan 14, 2025
  89. in test/functional/mining_mainnet.py:90 in dec5f8ba8f outdated 
    85+        block.nVersion = 0x20000000
86+        block.hashPrevBlock = int(prev_hash, 16)
87+        block.nTime = blocks['timestamps'][height - 1]
88+        block.nBits = DIFF_1_N_BITS
89+        block.nNonce = blocks['nonces'][height - 1]
90+        block.vtx = copy.deepcopy(txs)
    Sjors commented at 8:55 am on January 14, 2025:
    dec5f8ba8ff579e3e708079dde8a7ff732884479: @maflcko I’m puzzled by the need to deepcopy here, but if I just do block.vtx = txs then in the next call to mine, without passing any transactions in, txs will have the coinbase of the previous block in it.
    Sjors commented at 9:00 am on January 14, 2025:
    Well the linter forced me to change the txs default to None, which “solved” the issue.
  90. Sjors force-pushed on Jan 14, 2025
  91. DrahtBot added the label CI failed on Jan 14, 2025
  92. DrahtBot commented at 9:00 am on January 14, 2025: contributor

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

    Try to run the tests locally, according to the documentation. However, a CI failure may still 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.

  93. DrahtBot removed the label CI failed on Jan 14, 2025
  94. in src/rpc/blockchain.cpp:436 in 8502e6ea5a outdated 
    431@@ -431,7 +432,9 @@ static RPCHelpMan getdifficulty()
432 {
433     return RPCHelpMan{"getdifficulty",
434                 "\nReturns the proof-of-work difficulty as a multiple of the minimum difficulty.\n",
435-                {},
436+                {
437+                    {"next", RPCArg::Type::BOOL, RPCArg::Default{false}, "difficulty for the next block, if found now"},
    ismaelsadeeq commented at 10:27 pm on January 14, 2025:
    @Sjors what do you mean by “if found now” here and the other places?
    Sjors commented at 8:48 am on January 15, 2025:
    In testnet3 and testnet4 the difficulty depends on the timestamp. After 20 minutes it drops to 1. These methods derive difficulty using the current timestamp.
    maflcko commented at 8:56 am on January 15, 2025:
    I’d say that test-only docs shouldn’t be put in the real RPC docs for mainnet. If someone is running on t3/t4 they should be aware of the rules already. And if they are not aware, “if found now” doesn’t help to explain the rules to them.
    Sjors commented at 9:02 am on January 15, 2025:
    Ok, I dropped “if found now”.
  95. ismaelsadeeq commented at 10:33 pm on January 14, 2025: member

    Concept ACK
    I think the added feature here is useful to miners!

    The consensus changes involve splitting CheckProofOfWorkImpl into two functions:

    1. DeriveTarget, which returns the proof-of-work target given a compact target.
    2. CheckProofOfWorkImpl, which performs its normal check to verify whether a block hash satisfies the proof-of-work requirements.

    This enables the usage of DeriveTarget in other places.

    The large diff is due to an alternate mainnet chain data used in test 054bbceded4515a2cef7f247aca576f0c4413205.

  96. consensus: add DeriveTarget() to pow.h 
    Split CheckProofOfWorkImpl() to introduce a helper function
DeriveTarget() which converts the nBits value to the target.

The function takes pow_limit as an argument so later commits can
avoid having to pass ChainstateManager through the call stack.

Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
    21e31d7a31
  97. build: move pow and chain to bitcoin_common 
    The next commit needs pow.cpp in rpc/util.cpp.
    1e5e9b92ef
  98. rpc: add nBits to getmininginfo 
    Also expands nBits test coverage.
    4734734888
  99. test: use REGTEST_N_BITS in feature_block 2be8b47cc9
  100. rpc: gettarget f0e597546b
  101. Add target to getblock(header) in RPC and REST 8d467323ff
  102. rpc: add target to getmininginfo result f4f5ecea5d
  103. rpc: add next to getdifficulty and gettarget 
    Obtain the difficulty / target for the next block without having to call getblocktemplate.
    d7016e27f9
  104. rpc: add target and bits to getblockchaininfo a0d569b981
  105. rpc: add target and bits to getchainstates 1219f577a4
  106. rpc: add next to getmininginfo 59a8e300da
  107. Use OP_0 for BIP34 padding in signet and tests 
    For blocks 1 through 15 the script_BIP34_coinbase_height appends OP_1
to comply with BIP34 and avoid bad-cb-length.

This is inconsistent with BlockAssembler::CreateNewBlock() which adds
OP_0 instead.

The utxo_total_supply fuzzer and MinerTestingSetup::Block also use OP_0.

Changing it is required to import the test vectors in the next commit.

It also ensures the test vectors can be regenerated using the CPU miner
at https://github.com/pooler/cpuminer without patches (it uses OP_0).

The same helper is used by the signet miner, so this will impact newly
bootstrapped signets.
    75342f7e19
  108. Sjors force-pushed on Jan 15, 2025
  109. in test/functional/test_framework/blocktools.py:132 in 75342f7e19 outdated 
    127@@ -128,8 +128,8 @@ def add_witness_commitment(block, nonce=0):
128 def script_BIP34_coinbase_height(height):
129     if height <= 16:
130         res = CScriptOp.encode_op_n(height)
131-        # Append dummy to increase scriptSig size above 2 (see bad-cb-length consensus rule)
132-        return CScript([res, OP_1])
    ryanofsky commented at 9:05 pm on January 15, 2025:

    In commit “Use OP_0 for BIP34 padding in signet and tests” (75342f7e19cbbae083c2a3561f2c2cefd4e2968a)

    Do we know any reason why this code was using OP_1 instead of OP_0 previously? Any possible advantages to using OP_1? It looks like this code was introduced in #16363

    Sjors commented at 8:47 am on January 16, 2025:
    I don’t know. @maflcko?
    maflcko commented at 9:21 am on January 16, 2025:
    It is a dummy value, so any value that passes can be used.
  110. in test/functional/mining_mainnet.py:121 in a70f967bda outdated 
    116+                prev_hash = self.mine(i + 1, prev_hash, blocks, node)
117+
118+        assert_equal(node.getblockcount(), 2014)
119+
120+        # For the last block of the retarget period, check that previously generated
121+        # coins are spendable.
    ryanofsky commented at 9:31 pm on January 15, 2025:

    In commit “test: check difficulty adjustment using alternate mainnet” (a70f967bda5a0998a2b5a851cd32d969b021df15)

    This seems ok, but I think I don’t understand why it is useful to test that coins are spendable. It seems like something besides the point of the test (at least as stated in the description), which is to test difficulty retargeting functions on the main chain that can’t be tested on the regtest chain. I wonder if this is spending test is checking something that existing tests are not already checking for?

    More generally, I think it would be good to update description at top of this file to explain what things this test is trying to check for and cover. IMO, it is hard to parse right now because there is so much detail about how the test data file is generated and not as much information about the test itself. I think it would be better to move detailed information about how the data file is generated to a README file in the data directory, and only include more essential information and a link here.

    Sjors commented at 8:51 am on January 16, 2025:

    This seems ok, but I think I don’t understand why it is useful to test that coins are spendable.

    It’s basically checking against a bug I introduced in an earlier version. Initially I used taproot addresses for the coinbase which are unspendable due to unexpected-witness.

    However it might be simpler if I drop it and just leave a separate commit as a comment in the PR, if someone needs it later.

  111. ryanofsky approved
  112. ryanofsky commented at 9:39 pm on January 15, 2025: contributor
    Code review ACK 712e3ffde5fa4fd26c0f217942b4a289cf37f361. Main change since last review is mining_testnet.py test being replaced by mining_mainnet.py test with a much smaller data file. It was also rebased and there were small documentation changes and string format tweak in mining_mainnet.py
  113. DrahtBot requested review from tdb3 on Jan 15, 2025
  114. DrahtBot requested review from ismaelsadeeq on Jan 15, 2025
  115. tdb3 approved
  116. tdb3 commented at 3:05 am on January 16, 2025: contributor

    code review re ACK 712e3ffde5fa4fd26c0f217942b4a289cf37f361

    Since last review, the test was updated to use spendable coinbase UTXOs.

  117. Sjors force-pushed on Jan 16, 2025
  118. Sjors commented at 9:24 am on January 16, 2025: member

    I dropped the test code that proves the coinbase outputs are spendable. For posterity, it’s in https://github.com/Sjors/bitcoin/commit/ac516671b1846dbc40bea23c59399dbd7ec9b76b.

    Added test/functional/data/README.md to explain how mainnet_alt.json was generated.

  119. in test/functional/mining_mainnet.py:39 in 90b6c189d9 outdated 
    36+import os
37+
38+# Derived from first BIP32 test vector master key:
39+# Use pkh() because tr() outputs at low heights are not spendable (unexpected-witness)
40+COINBASE_OUTPUT_DESCRIPTOR="pkh(xprv9s21ZrQH143K3QTDL4LXw2F7HEK3wJUD2nW2nRk4stbPy6cq3jPPqjiChkVvvNKmPGJxWUtg6LnF5kejMRNNU3TGtRBeJgk33yuGBxrMPHi/44h/0h/0h/<0;1>/*)#fkjtr0yn"
41+COINBASE_SCRIPT_PUBKEY="76a914eadbac7f36c37e39361168b7aaee3cb24a25312d88ac"
    ryanofsky commented at 4:04 pm on January 16, 2025:

    In commit “test: check difficulty adjustment using alternate mainnet” (90b6c189d9b18203e119a5f3478f7b0da7d71392)

    Note: this seems ok, but I guess it could be simplified a little since COINBASE_OUTPUT_DESCRIPTOR is unused, and no spending is done anymore.

    In case you do want to bring back the spending test that seems fine if it’s explained a little. I wasn’t necessarily asking to remove it before, but was just confused by why it was there and a little overwhelmed with the details chain generation in the test description. Keeping this is as or simplifying more also both seem fine to me.

    Sjors commented at 4:25 pm on January 16, 2025:

    I’m fine with leaving it out.

    I also dropped the COINBASE_OUTPUT_DESCRIPTOR constant and moved the descriptor itself to the README.

  120. ryanofsky approved
  121. ryanofsky commented at 4:07 pm on January 16, 2025: contributor
    Code review ACK 39eabfa433e0bb9d85c67c6f9ce74c1570a21417. Since last review just simplified the test and clarified documentation a bit (thanks!)
  122. DrahtBot requested review from tdb3 on Jan 16, 2025
  123. test: check difficulty adjustment using alternate mainnet e0862b130c
  124. test: use DIFF_1_N_BITS in tool_signet_miner 8eb1b8d4e0
  125. doc: add release notes 
    Co-Authored-By: tdb3 <106488469+tdb3@users.noreply.github.com>
    cb305da72d
  126. Sjors force-pushed on Jan 16, 2025
  127. ryanofsky approved
  128. ryanofsky commented at 5:18 pm on January 16, 2025: contributor
    Code review ACK cb305da72df501991a6700cd20be79dde4591184, since last review moving coinbase key information from the test to the data readme file. I think this change makes both easier to understand now.
  129. tdb3 approved
  130. tdb3 commented at 3:02 am on January 17, 2025: contributor

    Code review re ACK cb305da72df501991a6700cd20be79dde4591184

    Nice cleanup. It seems reasonable to have removed the spend test.

    git range-diff e7c479495509c068215b73f6df070af2d406ae15..712e3ffde5fa4fd26c0f217942b4a289cf37f361 e7c479495509c068215b73f6df070af2d406ae15..cb305da72df501991a6700cd20be79dde4591184


Sjors DrahtBot tdb3 ryanofsky maflcko ismaelsadeeq


ismaelsadeeq

Labels
RPC/REST/ZMQ Consensus

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