Skip to content

Support Testnet4 Network#2945

Merged
apoelstra merged 2 commits intorust-bitcoin:masterfrom
BinChengZhao:testnet4
Oct 7, 2024
Merged

Support Testnet4 Network#2945
apoelstra merged 2 commits intorust-bitcoin:masterfrom
BinChengZhao:testnet4

Conversation

@BinChengZhao
Copy link
Contributor

Fixed: #2749

  1. Adjust Network to support Testnet4. (Based on Support for testnet4 #2749 (comment) as an implementation idea.)

  2. Handle conflicting procedure macros and declaration macros, since the original macros can't handle the new Network::Testnet(TestnetVersion) structure properly, and make internal implementations compatible with previous versions (e.g., testnet for Testnet3, testnet4 for Testnet4, to minimize user-side effects).

  3. add Testnet4 related Params, Block.

  4. optimize compatibility test cases.

  5. Regarding fn: genesis_block, calling bitcoin_genesis_tx with any Network type internally gets a reasonable merkle_root, but not Testnet4. (By comparison, I confirmed that the built-in transaction data provided is correct, and combined with the fixed merkle_root of Testnet4 the correct block hash can be computed, which is not possible if the transaction data is incorrect.) I'd appreciate it if some developer could guide me on what the problem is, and I'll work on it.

Of course, all these changes are based on #2749 , and the consensus of the discussion, if you have any comments, please leave a message, I will actively improve, and promote the support of Testnet4.

@github-actions github-actions bot added C-bitcoin PRs modifying the bitcoin crate test labels Jul 1, 2024
@coveralls
Copy link

coveralls commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9740234288

Details

  • 157 of 171 (91.81%) changed or added relevant lines in 6 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.06%) to 83.065%

Changes Missing Coverage Covered Lines Changed/Added Lines %
bitcoin/src/address/mod.rs 3 4 75.0%
bitcoin/src/consensus/params.rs 3 4 75.0%
bitcoin/src/p2p/mod.rs 18 19 94.74%
bitcoin/src/blockdata/constants.rs 47 49 95.92%
bitcoin/src/network.rs 85 94 90.43%
Files with Coverage Reduction New Missed Lines %
bitcoin/src/network.rs 1 84.15%
bitcoin/src/blockdata/constants.rs 1 96.1%
Totals Coverage Status
Change from base Build 9736483684: 0.06%
Covered Lines: 19625
Relevant Lines: 23626

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9742783239

Details

  • 143 of 156 (91.67%) changed or added relevant lines in 6 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.05%) to 83.058%

Changes Missing Coverage Covered Lines Changed/Added Lines %
bitcoin/src/address/mod.rs 3 4 75.0%
bitcoin/src/consensus/params.rs 3 4 75.0%
bitcoin/src/blockdata/constants.rs 46 48 95.83%
bitcoin/src/network.rs 86 95 90.53%
Files with Coverage Reduction New Missed Lines %
bitcoin/src/network.rs 1 84.15%
bitcoin/src/blockdata/constants.rs 1 96.07%
Totals Coverage Status
Change from base Build 9736483684: 0.05%
Covered Lines: 19615
Relevant Lines: 23616

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9742953216

Details

  • 143 of 156 (91.67%) changed or added relevant lines in 6 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.05%) to 83.058%

Changes Missing Coverage Covered Lines Changed/Added Lines %
bitcoin/src/address/mod.rs 3 4 75.0%
bitcoin/src/consensus/params.rs 3 4 75.0%
bitcoin/src/blockdata/constants.rs 46 48 95.83%
bitcoin/src/network.rs 86 95 90.53%
Files with Coverage Reduction New Missed Lines %
bitcoin/src/network.rs 1 84.15%
bitcoin/src/blockdata/constants.rs 1 96.07%
Totals Coverage Status
Change from base Build 9736483684: 0.05%
Covered Lines: 19615
Relevant Lines: 23616

💛 - Coveralls

@BinChengZhao
Copy link
Contributor Author

  • The original declaration macro generate_network_magic_conversion was split into two patterns, Because there was a risk of conflict in doing two code generations in one patten.

  • Since compound Enum cannot use as usize as the index of slice, From<Network> for usize is implemented for Network;

  • Because into() -> size can no longer be executed in const fn, so the signature of a function is adjusted.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please squash the commits, some changes are hard to review this way.

@BinChengZhao
Copy link
Contributor Author

Please squash the commits, some changes are hard to review this way.

I will pay attention to this in squash commit

@coveralls
Copy link

coveralls commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9747218577

Details

  • 143 of 156 (91.67%) changed or added relevant lines in 6 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.05%) to 83.058%

Changes Missing Coverage Covered Lines Changed/Added Lines %
bitcoin/src/address/mod.rs 3 4 75.0%
bitcoin/src/consensus/params.rs 3 4 75.0%
bitcoin/src/blockdata/constants.rs 46 48 95.83%
bitcoin/src/network.rs 86 95 90.53%
Files with Coverage Reduction New Missed Lines %
bitcoin/src/network.rs 1 84.15%
bitcoin/src/blockdata/constants.rs 1 96.07%
Totals Coverage Status
Change from base Build 9745227269: 0.05%
Covered Lines: 19615
Relevant Lines: 23616

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 1, 2024

Pull Request Test Coverage Report for Build 9747894544

Details

  • 138 of 157 (87.9%) changed or added relevant lines in 6 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.05%) to 83.052%

Changes Missing Coverage Covered Lines Changed/Added Lines %
bitcoin/src/address/mod.rs 3 4 75.0%
bitcoin/src/consensus/params.rs 3 4 75.0%
bitcoin/src/blockdata/constants.rs 49 55 89.09%
bitcoin/src/network.rs 78 89 87.64%
Files with Coverage Reduction New Missed Lines %
bitcoin/src/blockdata/constants.rs 1 94.92%
bitcoin/src/network.rs 2 84.31%
Totals Coverage Status
Change from base Build 9745227269: 0.05%
Covered Lines: 19611
Relevant Lines: 23613

💛 - Coveralls

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to find the hash problem for a bit but couldn't so far.

@coveralls
Copy link

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9754139109

Details

  • 138 of 157 (87.9%) changed or added relevant lines in 6 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.05%) to 83.052%

Changes Missing Coverage Covered Lines Changed/Added Lines %
bitcoin/src/address/mod.rs 3 4 75.0%
bitcoin/src/consensus/params.rs 3 4 75.0%
bitcoin/src/blockdata/constants.rs 49 55 89.09%
bitcoin/src/network.rs 78 89 87.64%
Files with Coverage Reduction New Missed Lines %
bitcoin/src/blockdata/constants.rs 1 94.92%
bitcoin/src/network.rs 2 84.31%
Totals Coverage Status
Change from base Build 9745227269: 0.05%
Covered Lines: 19611
Relevant Lines: 23613

💛 - Coveralls

@BinChengZhao
Copy link
Contributor Author

BinChengZhao commented Jul 2, 2024

I tried to find the hash problem for a bit but couldn't so far.

I tried to find the hash problem for a bit but couldn't so far.

I've made several attempts so far and I haven't made any progress.

Now in doubt some of 'script::Builder::push_slice' internal logic, because Testnet4 provides a new 'genesis_msg' : 03/May/2024 000000000000000000001ebd58c244970b3aa9d783bb001011fbe8ea8e98e00e (len is 76) might push different branches. But I don't quite understand the internal logic at the moment, it's just conjecture.

Trying some hack didn't seem to work either

@Kixunil
Copy link
Collaborator

Kixunil commented Jul 2, 2024

I don't think it's that. Perhaps it'll be faster if you debug-serialize the transaction and compare it to the one in Core repo.

Also important note: while that Core PR is not merged and released this PR cannot be merged.

@Kixunil Kixunil changed the title Support Testnet4 Network in rust-bitcoin Library Support Testnet4 Network Jul 2, 2024
@BinChengZhao
Copy link
Contributor Author

Also important note: while that Core PR is not merged and released this PR cannot be merged.

I understand the need to wait for progress on bitcoin Core

@BinChengZhao
Copy link
Contributor Author

I don't think it's that. Perhaps it'll be faster if you debug-serialize the transaction and compare it to the one in Core repo.

Ok I try to debug

Since I started writing Rust, C++ has almost forgotten, debugging is also a knowledge review :)

@coveralls

This comment was marked as outdated.

@coveralls
Copy link

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9764933554

Details

  • 133 of 152 (87.5%) changed or added relevant lines in 6 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.04%) to 83.047%

Changes Missing Coverage Covered Lines Changed/Added Lines %
bitcoin/src/address/mod.rs 3 4 75.0%
bitcoin/src/consensus/params.rs 3 4 75.0%
bitcoin/src/blockdata/constants.rs 44 50 88.0%
bitcoin/src/network.rs 78 89 87.64%
Files with Coverage Reduction New Missed Lines %
bitcoin/src/blockdata/constants.rs 1 94.76%
bitcoin/src/network.rs 2 84.31%
Totals Coverage Status
Change from base Build 9763162782: 0.04%
Covered Lines: 19604
Relevant Lines: 23606

💛 - Coveralls

@BinChengZhao
Copy link
Contributor Author

I don't think it's that. Perhaps it'll be faster if you debug-serialize the transaction and compare it to the one in Core repo.

Also important note: while that Core PR is not merged and released this PR cannot be merged.

After a lot of cross-debug with the bitcoin-Core repo, I have found the problem. The new testnet4 uses a compressed public key. After I have processed the corresponding logic, there is no hack

Thanks for the tip

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good now, I'll be happy to ACK once Core is released.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"test4" => Testnet(TestnetVersion::V4),
"testnet4" => Testnet(TestnetVersion::V4),

bitcoind node will return testnet4 and serialization will fail, you should use actual value instead of test4

image

curl --location --request POST 'http://127.0.0.1:48332' \
--header 'Authorization: Basic ...snip...' \
--header 'Content-Type: application/json' \
--data-raw '{
    "method": "getblockchaininfo",
    "params": []
}'
{
    "result": {
        "chain": "testnet4",
        "blocks": 36409,
        "headers": 36409,
        "bestblockhash": "0000000000000058e9f909a8d5a86afc8f58d39b661b02f07433d4cae8b6481d",
        "difficulty": 44472500.68822794,
        "time": 1721977342,
        "mediantime": 1721974212,
        "verificationprogress": 1,
        "initialblockdownload": false,
        "chainwork": "000000000000000000000000000000000000000000000048dcdd24a9fc06ef13",
        "size_on_disk": 284698607,
        "pruned": false,
        "warnings": [
            "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
        ]
    },
    "error": null
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Insane, so the Core devs decided to make it as inconsistent as possible?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions
Copy link

github-actions bot commented Oct 3, 2024

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

@github-actions
Copy link

github-actions bot commented Oct 3, 2024

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

@BinChengZhao
Copy link
Contributor Author

I'm a bit late to get to reviewing this, all my questions can be seen as nits and need not be addressed before merging this.

Thank you very much for your valuable suggestions, I have made the code synchronous changes related to the suggestions

@github-actions
Copy link

github-actions bot commented Oct 3, 2024

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

apoelstra
apoelstra previously approved these changes Oct 3, 2024
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1ef2ab9b62c15602338d5cf97d442805ef614bee successfully ran local tests

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sticking with this, with the comment removed: ACK 1ef2ab9b62c15602338d5cf97d442805ef614bee

@github-actions
Copy link

github-actions bot commented Oct 5, 2024

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

@BinChengZhao
Copy link
Contributor Author

Thanks for sticking with this, with the comment removed: ACK 1ef2ab9

The corresponding comments have been removed, thank you for your valuable time review

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2424b65

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2424b65 successfully ran local tests

@apoelstra apoelstra merged commit 31f1a12 into rust-bitcoin:master Oct 7, 2024
@apoelstra
Copy link
Member

Let's try to backport this.

@junderw
Copy link
Contributor

junderw commented Oct 9, 2024

Let's try to backport this.

Looks like this is going into 0.33?

0.32 and 0.31? How far should we backport?

@apoelstra
Copy link
Member

Just to 0.32 is fine.

macols77 added a commit to macols77/rust-bitcoin that referenced this pull request Oct 18, 2024
2424b65 feat: rust-bitcoin supports testnet4 (BinChengZhao)
afa91a2 Introduce TestnetVersion enum with only TestnetV3 (BinChengZhao)

Pull request description:

  Fixed: rust-bitcoin#2749

  1. Adjust `Network` to support `Testnet4`. (Based on rust-bitcoin#2749 (comment) as an implementation idea.)

  2. Handle conflicting procedure macros and declaration macros, since the original macros can't handle the new `Network::Testnet(TestnetVersion)` structure properly, and make internal implementations compatible with previous versions (e.g., `testnet` for `Testnet3`, `testnet4` for `Testnet4`, to minimize user-side effects).

  3. add `Testnet4` related Params, Block.

  4. optimize compatibility test cases.

  5. Regarding `fn: genesis_block`, calling `bitcoin_genesis_tx` with any Network type internally gets a reasonable `merkle_root`, but not `Testnet4`. (By comparison, I confirmed that the built-in transaction data provided is correct, and combined with the fixed `merkle_root` of `Testnet4` the correct block hash can be computed, which is not possible if the transaction data is incorrect.) **I'd appreciate it if some developer could guide me on what the problem is, and I'll work on it.**

  Of course, all these changes are based on rust-bitcoin#2749 , and the consensus of the discussion, if you have any comments, please leave a message, I will actively improve, and promote the support of `Testnet4`.

ACKs for top commit:
  tcharding:
    ACK 2424b65
  apoelstra:
    ACK 2424b65 successfully ran local tests

Tree-SHA512: a501f0a320460afdad2eb12ef6ff315b6357b7779dfe7c7028ea090da567019c947599006a06b3834265db1481129752f015629ef72fea6f862d01323ce9d024
@tcharding
Copy link
Member

Backported in #3453

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API break This PR requires a version bump for the next release C-bitcoin PRs modifying the bitcoin crate Needs Backport test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for testnet4

9 participants