💬 151henry151 commented on pull request "Check required interfaces before generating manpages":
(https://github.com/bitcoin/bitcoin/pull/33828#issuecomment-3514498164)
Thanks for taking a look, fanquake.
I noticed #33085 had gone quiet for a little over three weeks with the review concerns still open, so I put this together.
In #33085 the script pulls component flags from `test/config.ini` (adding checks for CLI, chainstate, fuzz, USDT tracepoints, etc.), but that file only exists when tests are configured and most of those toggles don’t affect the manpages, so the script breaks in release-style builds. This branch reads `CMakeCache.txt`, the authorita
...
(https://github.com/bitcoin/bitcoin/pull/33828#issuecomment-3514498164)
Thanks for taking a look, fanquake.
I noticed #33085 had gone quiet for a little over three weeks with the review concerns still open, so I put this together.
In #33085 the script pulls component flags from `test/config.ini` (adding checks for CLI, chainstate, fuzz, USDT tracepoints, etc.), but that file only exists when tests are configured and most of those toggles don’t affect the manpages, so the script breaks in release-style builds. This branch reads `CMakeCache.txt`, the authorita
...
💬 kevkevinpal commented on issue "`test_kernel` fails to build on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3514905241)
If you want to reproduce using docker here is the docker file
```
FROM ubuntu:22.04
RUN apt update && \
apt install -y git build-essential cmake pkgconf python3 libevent-dev libboost-dev libsqlite3-dev
WORKDIR /bitcoin
RUN git clone https://github.com/bitcoin/bitcoin.git .
RUN cmake -B build -DENABLE_IPC=OFF -DBUILD_KERNEL_LIB=ON && \
cmake --build build -j $(nproc)
```
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3514905241)
If you want to reproduce using docker here is the docker file
```
FROM ubuntu:22.04
RUN apt update && \
apt install -y git build-essential cmake pkgconf python3 libevent-dev libboost-dev libsqlite3-dev
WORKDIR /bitcoin
RUN git clone https://github.com/bitcoin/bitcoin.git .
RUN cmake -B build -DENABLE_IPC=OFF -DBUILD_KERNEL_LIB=ON && \
cmake --build build -j $(nproc)
```
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-3515363192)
CI fails with:
```
Bech32 address decoded with error: Invalid character or mixed case == Bech32(m) address decoded with error: Invalid character or mixed case
```
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-3515363192)
CI fails with:
```
Bech32 address decoded with error: Invalid character or mixed case == Bech32(m) address decoded with error: Invalid character or mixed case
```
💬 maflcko commented on issue "`test_kernel` fails to build on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3515396269)
Please remove the include. See https://github.com/bitcoin/bitcoin/actions/runs/19176694315/job/54823183155?pr=33810#step:9:18532 :
```
/home/admin/actions-runner/_work/_temp/src/test/kernel/test_kernel.cpp should remove these lines:
- #include <boost/test/included/unit_test.hpp> // lines 9-9
- #include <cstdlib> // lines 15-15
- #include <format> // lines 17-17
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3515396269)
Please remove the include. See https://github.com/bitcoin/bitcoin/actions/runs/19176694315/job/54823183155?pr=33810#step:9:18532 :
```
/home/admin/actions-runner/_work/_temp/src/test/kernel/test_kernel.cpp should remove these lines:
- #include <boost/test/included/unit_test.hpp> // lines 9-9
- #include <cstdlib> // lines 15-15
- #include <format> // lines 17-17
🤔 hodlinator reviewed a pull request: "test, refactor: Embedded ASmap selected preparatory work"
(https://github.com/bitcoin/bitcoin/pull/33026#pullrequestreview-3441618331)
Reviewed 5bb456c92bbd27ebbba1773832b051968f162b8a
Thanks for providing a less intimidating PR as a stepping stone for reviewers new to this domain like myself.
---
PR description still mentions 4 commits but only 3 are currently present (after https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2429247548).
(https://github.com/bitcoin/bitcoin/pull/33026#pullrequestreview-3441618331)
Reviewed 5bb456c92bbd27ebbba1773832b051968f162b8a
Thanks for providing a less intimidating PR as a stepping stone for reviewers new to this domain like myself.
---
PR description still mentions 4 commits but only 3 are currently present (after https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2429247548).
💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509317566)
nit in 5a63a5a1fdf84625c411acb72fa51e12f9a8b067:
New code should use more modern `constexpr std::byte`s (and optionally `reserve()` vector):
```diff
BOOST_AUTO_TEST_CASE(asmap_test_vectors)
{
// Randomly generated encoded ASMap with 128 ranges, up to 20-bit AS numbers.
- static const auto ASMAP_DATA =
+ constexpr auto ASMAP_DATA{
"fd38d50f7d5d665357f64bba6bfc190d6078a7e68e5d3ac032edf47f8b5755f87881bfd3633d9aa7c1fa279b3"
"6fe26c63bbc9de44e0f04e5a382d8e1cddbe1c26653b
...
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509317566)
nit in 5a63a5a1fdf84625c411acb72fa51e12f9a8b067:
New code should use more modern `constexpr std::byte`s (and optionally `reserve()` vector):
```diff
BOOST_AUTO_TEST_CASE(asmap_test_vectors)
{
// Randomly generated encoded ASMap with 128 ranges, up to 20-bit AS numbers.
- static const auto ASMAP_DATA =
+ constexpr auto ASMAP_DATA{
"fd38d50f7d5d665357f64bba6bfc190d6078a7e68e5d3ac032edf47f8b5755f87881bfd3633d9aa7c1fa279b3"
"6fe26c63bbc9de44e0f04e5a382d8e1cddbe1c26653b
...
💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509429711)
nit:
```suggestion
const int64_t size{db_file.size()};
```
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509429711)
nit:
```suggestion
const int64_t size{db_file.size()};
```
💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509422876)
Why do we get 14?
Tried adding...
```C++
{
AutoFile test{raw_file("rb"), obfuscation};
BOOST_CHECK_EQUAL(test.size(), 14);
}
```
...just after this block before digging into the data but it fails with `size()` suddenly returning 7 (which would be what we expect: 2 vector compact size bytes + 2+3 bytes of data).
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509422876)
Why do we get 14?
Tried adding...
```C++
{
AutoFile test{raw_file("rb"), obfuscation};
BOOST_CHECK_EQUAL(test.size(), 14);
}
```
...just after this block before digging into the data but it fails with `size()` suddenly returning 7 (which would be what we expect: 2 vector compact size bytes + 2+3 bytes of data).
💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509435441)
This last `seek()` can be removed and all unit tests still pass (259 non-skipped functional tests all passed as well, no failures in my non-`--extended` run), could add a test that covers this so we don't spawn any mutation testing mutants.
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509435441)
This last `seek()` can be removed and all unit tests still pass (259 non-skipped functional tests all passed as well, no failures in my non-`--extended` run), could add a test that covers this so we don't spawn any mutation testing mutants.
💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509450577)
nit: Should be moved above `DataStream::GetMemoryUsage()` so it's kept together with other `AutoFile` methods. Maybe after `AutoFile::tell` to keep some consistency with header file.
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509450577)
nit: Should be moved above `DataStream::GetMemoryUsage()` so it's kept together with other `AutoFile` methods. Maybe after `AutoFile::tell` to keep some consistency with header file.
💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509464158)
remark: Good that it is non-`const` as it is not thread-safe. :+1:
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2509464158)
remark: Good that it is non-`const` as it is not thread-safe. :+1:
💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2511238194)
remark: Tried...
```shell
₿ printf fd38d50f7d5d665357f64bba6bfc190d6078a7e68e5d3ac032edf47f8b5755f87881bfd3633d9aa7c1fa279b36fe26c63bbc9de44e0f04e5a382d8e1cddbe1c26653bc939d4327f287e8b4d1f8aff33176787cb0ff7cb28e3fdaef0f8f47357f801c9f7ff7a99f7f9c9f99de7f3156ae00f23eb27a303bc486aa3ccc31ec19394c2f8a53dddea3cc56257f3b7e9b1f488be9c1137db823759aa4e071eef2e984aaf97b52d5f88d0f373dd190fe45e06efef1df7278be680a73a74c76db4dd910f1d30752c57fe2bc9f079f1a1e1b036c2a69219f11c5e11980a3fa51f4f82d36373de73b1863a8c
...
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2511238194)
remark: Tried...
```shell
₿ printf fd38d50f7d5d665357f64bba6bfc190d6078a7e68e5d3ac032edf47f8b5755f87881bfd3633d9aa7c1fa279b36fe26c63bbc9de44e0f04e5a382d8e1cddbe1c26653bc939d4327f287e8b4d1f8aff33176787cb0ff7cb28e3fdaef0f8f47357f801c9f7ff7a99f7f9c9f99de7f3156ae00f23eb27a303bc486aa3ccc31ec19394c2f8a53dddea3cc56257f3b7e9b1f488be9c1137db823759aa4e071eef2e984aaf97b52d5f88d0f373dd190fe45e06efef1df7278be680a73a74c76db4dd910f1d30752c57fe2bc9f079f1a1e1b036c2a69219f11c5e11980a3fa51f4f82d36373de73b1863a8c
...
💬 hodlinator commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2511202729)
Instead of implementing `AutoFile::size()`, did you consider using `fs::file_size()` as we do in a few other places?
(https://github.com/bitcoin/bitcoin/pull/33026#discussion_r2511202729)
Instead of implementing `AutoFile::size()`, did you consider using `fs::file_size()` as we do in a few other places?
🤔 Sjors reviewed a pull request: "Enhanced error messages for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260#pullrequestreview-3446684328)
Thanks for splitting base58 and bech32 stuff into separate commits. I think that 04d5d4acab4259505654a506c838fecb29fdb88d _Base58 decoding logic + bech32 decoding network awareness_ could itself be split, see inline comment from @l0rinc, since it's still contains a lot of bech32 changes despite the commit name.
My main other suggestion is to drop the first commit (and don't add the network name to error messages).
See inline comment for how to fix the test.
(https://github.com/bitcoin/bitcoin/pull/27260#pullrequestreview-3446684328)
Thanks for splitting base58 and bech32 stuff into separate commits. I think that 04d5d4acab4259505654a506c838fecb29fdb88d _Base58 decoding logic + bech32 decoding network awareness_ could itself be split, see inline comment from @l0rinc, since it's still contains a lot of bech32 changes despite the commit name.
My main other suggestion is to drop the first commit (and don't add the network name to error messages).
See inline comment for how to fix the test.
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513222433)
In f8513e38b369889df0b402f06d67fcf656495d53 _[Functional] Introduce muti network testing_: the extra `(m)` breaks the test
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513222433)
In f8513e38b369889df0b402f06d67fcf656495d53 _[Functional] Introduce muti network testing_: the extra `(m)` breaks the test
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513235396)
In f8513e38b369889df0b402f06d67fcf656495d53 _[Functional] Introduce muti network testing_:
The elegant solution imo is to move these test vectors into a JSON file, see e.g. `rpc_getblockstats.py`.
But I think the current approach is fine. You could also duplicate the body of `test_validateaddress_on_network` into `test_validateaddress_mainnet` and `test_validateaddress_signet` rather than copying the constants. Each for loop could then be preceded with `self.log.debug("Valid mainnet addre
...
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513235396)
In f8513e38b369889df0b402f06d67fcf656495d53 _[Functional] Introduce muti network testing_:
The elegant solution imo is to move these test vectors into a JSON file, see e.g. `rpc_getblockstats.py`.
But I think the current approach is fine. You could also duplicate the body of `test_validateaddress_on_network` into `test_validateaddress_mainnet` and `test_validateaddress_signet` rather than copying the constants. Each for loop could then be preceded with `self.log.debug("Valid mainnet addre
...
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513250539)
Nit for c974f892c7b6a61630c555f08ec201d911d5fd62 _Bech32 decoding logic_: unrelated whitespace change and random looking `asdas` in the commit message.
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513250539)
Nit for c974f892c7b6a61630c555f08ec201d911d5fd62 _Bech32 decoding logic_: unrelated whitespace change and random looking `asdas` in the commit message.
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513216866)
In f8513e38b369889df0b402f06d67fcf656495d53 _[Functional] Introduce muti network testing_:
These were disabled in #27727, IIUC in favour the official test vectors. Seems fine to delete them. If they're actually interesting, then they can be restored and added to the BIP.
(nit: typo in commit message)
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513216866)
In f8513e38b369889df0b402f06d67fcf656495d53 _[Functional] Introduce muti network testing_:
These were disabled in #27727, IIUC in favour the official test vectors. Seems fine to delete them. If they're actually interesting, then they can be restored and added to the BIP.
(nit: typo in commit message)
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513196144)
In 7b71e6e056f5567152097d5c9a1a8e2f59c1b661 _Include Base58 encoded prefixes in chainparams_: this commit only adds base58 prefixes, silent payments will use bech32m.
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513196144)
In 7b71e6e056f5567152097d5c9a1a8e2f59c1b661 _Include Base58 encoded prefixes in chainparams_: this commit only adds base58 prefixes, silent payments will use bech32m.
💬 Sjors commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513186197)
In 055dbaed69a2577b967fae3d9b7646ef3b01a254 _Added chaintype user-facing string display_:
This is used as follows in later commits:
> `Invalid Base58 %s address.`
In the original code it would not specify the network:
> `Invalid or unsupported Base58-encoded address.`
I don't think it's very useful to add the network name to address. Certainly not for mainnet, because most users have no idea (and no need to know) about test networks.
My suggestion would be drop this helper enti
...
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2513186197)
In 055dbaed69a2577b967fae3d9b7646ef3b01a254 _Added chaintype user-facing string display_:
This is used as follows in later commits:
> `Invalid Base58 %s address.`
In the original code it would not specify the network:
> `Invalid or unsupported Base58-encoded address.`
I don't think it's very useful to add the network name to address. Certainly not for mainnet, because most users have no idea (and no need to know) about test networks.
My suggestion would be drop this helper enti
...