💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744062744)
Done.
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744062744)
Done.
💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744063063)
Adopted both suggestions, thanks!
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744063063)
Adopted both suggestions, thanks!
👍 tdb3 approved a pull request: "build: Fix / improve coverage scripts"
(https://github.com/bitcoin/bitcoin/pull/30772#pullrequestreview-2280663264)
cr re ACK d9fcbfc3727e06e6f57d4ab09861f3212d558426
(https://github.com/bitcoin/bitcoin/pull/30772#pullrequestreview-2280663264)
cr re ACK d9fcbfc3727e06e6f57d4ab09861f3212d558426
💬 maflcko commented on pull request "lint: Check for release note snippets in the wrong folder":
(https://github.com/bitcoin/bitcoin/pull/30812#discussion_r1744076957)
> using regex to match the name instead of string manipulations, repeating parts of the path and the file's structure (extension + dot);
Seems overkill to import a dependency to check whether a dot is present in a string view or not.
> match a slice of the files to return a `Result`; extract intermediary results for clarity;
Sure, I'll try that tomorrow.
> we could like use [fs::read_dir("doc/release-notes")](https://doc.rust-lang.org/std/fs/fn.read_dir.html) to iterate the fol
...
(https://github.com/bitcoin/bitcoin/pull/30812#discussion_r1744076957)
> using regex to match the name instead of string manipulations, repeating parts of the path and the file's structure (extension + dot);
Seems overkill to import a dependency to check whether a dot is present in a string view or not.
> match a slice of the files to return a `Result`; extract intermediary results for clarity;
Sure, I'll try that tomorrow.
> we could like use [fs::read_dir("doc/release-notes")](https://doc.rust-lang.org/std/fs/fn.read_dir.html) to iterate the fol
...
⚠️ fanquake opened an issue: "build: reproducibility issue with macOS Guix builds"
(https://github.com/bitcoin/bitcoin/issues/30815)
There seems to be a recurring (permissions related?) issue in the macOS guix builds, that is causing non-determinsm. It seems to have started since the CMake switchover. Copied in the related discussion / diff of comparing two outputs from #30743:
> EDIT: Probably something with the zipping again?
> Yea, looks like a permissions issue:
#### Comparing `bitcoin-556775408797-arm64-apple-darwin-unsigned.zip.fanquake` & `bitcoin-556775408797-arm64-apple-darwin-unsigned.zip.thecharlatan`
...
(https://github.com/bitcoin/bitcoin/issues/30815)
There seems to be a recurring (permissions related?) issue in the macOS guix builds, that is causing non-determinsm. It seems to have started since the CMake switchover. Copied in the related discussion / diff of comparing two outputs from #30743:
> EDIT: Probably something with the zipping again?
> Yea, looks like a permissions issue:
#### Comparing `bitcoin-556775408797-arm64-apple-darwin-unsigned.zip.fanquake` & `bitcoin-556775408797-arm64-apple-darwin-unsigned.zip.thecharlatan`
...
👍 l0rinc approved a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2280694965)
ACK b60013ed71b697bab98884aa475fb64ae7736c2e
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2280694965)
ACK b60013ed71b697bab98884aa475fb64ae7736c2e
💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744081326)
so these are basically roundtrip tests now, right?
These are very valuable (could even be a fuzz test now), but could we add back some of the hardcoded ones to make sure they're not just self-referentially correct (i.e. circular reasoning), i.e. something like:
```C++
BOOST_CHECK_EQUAL(UintToArith256(ZeroL), 0);
BOOST_CHECK_EQUAL(UintToArith256(OneL), 1);
BOOST_CHECK_EQUAL(ArithToUint256(0), ZeroL);
BOOST_CHECK_EQUAL(ArithToUint256(1), OneL);
BOOST_CHECK_EQUAL(ArithT
...
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744081326)
so these are basically roundtrip tests now, right?
These are very valuable (could even be a fuzz test now), but could we add back some of the hardcoded ones to make sure they're not just self-referentially correct (i.e. circular reasoning), i.e. something like:
```C++
BOOST_CHECK_EQUAL(UintToArith256(ZeroL), 0);
BOOST_CHECK_EQUAL(UintToArith256(OneL), 1);
BOOST_CHECK_EQUAL(ArithToUint256(0), ZeroL);
BOOST_CHECK_EQUAL(ArithToUint256(1), OneL);
BOOST_CHECK_EQUAL(ArithT
...
🤔 ismaelsadeeq reviewed a pull request: "test: Add explicit onion bind to p2p_permissions"
(https://github.com/bitcoin/bitcoin/pull/30805#pullrequestreview-2280706719)
Tested ACK e9ad9e01409eaa2009f75deb1cf834983c5a6137
I was able to recreate the issue locally when running the `p2p_permissions` test while `bitcoind` was running on `regtest`.
<details>
<summary>Before</summary>
```terminal
abubakarismail@Abubakars-MacBook-Pro bitcoin % build/test/functional/p2p_permissions.py
2024-09-01T12:48:15.964000Z TestFramework (INFO): PRNG seed is: 6689536799961214212
2024-09-01T12:48:15.964000Z TestFramework (INFO): Initializing test directory /var/fold
...
(https://github.com/bitcoin/bitcoin/pull/30805#pullrequestreview-2280706719)
Tested ACK e9ad9e01409eaa2009f75deb1cf834983c5a6137
I was able to recreate the issue locally when running the `p2p_permissions` test while `bitcoind` was running on `regtest`.
<details>
<summary>Before</summary>
```terminal
abubakarismail@Abubakars-MacBook-Pro bitcoin % build/test/functional/p2p_permissions.py
2024-09-01T12:48:15.964000Z TestFramework (INFO): PRNG seed is: 6689536799961214212
2024-09-01T12:48:15.964000Z TestFramework (INFO): Initializing test directory /var/fold
...
💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744104279)
> so these are basically roundtrip tests now, right?
I think `BOOST_CHECK_EQUAL(UintToArith256(ArithToUint256(arith)), arith);` is the only roundtrip?
> but could we add back some of the hardcoded ones to make sure they're not just self-referentially correct (i.e. circular reasoning):
Isn't that covered here? https://github.com/bitcoin/bitcoin/pull/30773/files#diff-d06d0fcd312512f190cc0d05450dd55973c60aeb8d57ed2d705e7beaa383f386R565-R568
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744104279)
> so these are basically roundtrip tests now, right?
I think `BOOST_CHECK_EQUAL(UintToArith256(ArithToUint256(arith)), arith);` is the only roundtrip?
> but could we add back some of the hardcoded ones to make sure they're not just self-referentially correct (i.e. circular reasoning):
Isn't that covered here? https://github.com/bitcoin/bitcoin/pull/30773/files#diff-d06d0fcd312512f190cc0d05450dd55973c60aeb8d57ed2d705e7beaa383f386R565-R568
💬 ryanofsky commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1744107968)
Followup points:
- I misunderstood "the build system may assign" to mean "cmake may assign" but actually it is referring to our own [ProcessConfigurations.cmake](https://github.com/bitcoin/bitcoin/blob/b8d2f58e06439e9523c02ca1729ff20cd0ba53ca/cmake/module/ProcessConfigurations.cmake#L145) file. So I think a more accurate comment than what I suggested would be "Set CMAKE_{C,CXX}\_FLAGS_DEBUG to empty so the default options normally used for debug builds do not override MSAN options in CMAKE_{C
...
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1744107968)
Followup points:
- I misunderstood "the build system may assign" to mean "cmake may assign" but actually it is referring to our own [ProcessConfigurations.cmake](https://github.com/bitcoin/bitcoin/blob/b8d2f58e06439e9523c02ca1729ff20cd0ba53ca/cmake/module/ProcessConfigurations.cmake#L145) file. So I think a more accurate comment than what I suggested would be "Set CMAKE_{C,CXX}\_FLAGS_DEBUG to empty so the default options normally used for debug builds do not override MSAN options in CMAKE_{C
...
💬 stickies-v commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1744124233)
Thanks for looking into it @marcofleon! If there are no operational concerns adding the test cases, I'm happy with it.
I guess if we keep this, the `uint256::FromHex()` asserts have become superfluous and could be removed. Shouldn't make a meaningful performance impact (since they're called much less often than the `FromUserHex()` asserts, so it's more of a thing to keep the test code lean.
<details>
<summary>git diff on 6fa4c92e4c</summary>
```diff
diff --git a/src/test/fuzz/hex.cpp
...
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1744124233)
Thanks for looking into it @marcofleon! If there are no operational concerns adding the test cases, I'm happy with it.
I guess if we keep this, the `uint256::FromHex()` asserts have become superfluous and could be removed. Shouldn't make a meaningful performance impact (since they're called much less often than the `FromUserHex()` asserts, so it's more of a thing to keep the test code lean.
<details>
<summary>git diff on 6fa4c92e4c</summary>
```diff
diff --git a/src/test/fuzz/hex.cpp
...
💬 stickies-v commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1744129399)
No I don't feel strongly about it, I just think this change is not nearly useful enough to warrant an extra 33LoC diff. Anyway, not worth arguing over, like I said it's not blocking.
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1744129399)
No I don't feel strongly about it, I just think this change is not nearly useful enough to warrant an extra 33LoC diff. Anyway, not worth arguing over, like I said it's not blocking.
👍 ryanofsky approved a pull request: "kernel: Create usable static kernel library"
(https://github.com/bitcoin/bitcoin/pull/30814#pullrequestreview-2280796157)
Code review ACK 192d71e312d0eab6cc1ce061edb34d9d338f7fec.
This seems like a reasonable approach, but I wonder if a more consistent way of doing this would just treat all the libraries the same as secp, installing them to the lib/ directory and listing them in libbitcoinkernel.pc. That way the secp library would not be an outlier, the TARGET_OBJECTS calls would not be needed, and build would be lighter, with each .o object file added to one library file .a instead of two.
(https://github.com/bitcoin/bitcoin/pull/30814#pullrequestreview-2280796157)
Code review ACK 192d71e312d0eab6cc1ce061edb34d9d338f7fec.
This seems like a reasonable approach, but I wonder if a more consistent way of doing this would just treat all the libraries the same as secp, installing them to the lib/ directory and listing them in libbitcoinkernel.pc. That way the secp library would not be an outlier, the TARGET_OBJECTS calls would not be needed, and build would be lighter, with each .o object file added to one library file .a instead of two.
💬 ryanofsky commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1744149228)
Not important, but it would probably make sense for both `disable_network` and `temporary_rollback` variables to be `std::optional` instead of `std::unique_ptr` since they aren't passed around it would simplify the way they are initialized.
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1744149228)
Not important, but it would probably make sense for both `disable_network` and `temporary_rollback` variables to be `std::optional` instead of `std::unique_ptr` since they aren't passed around it would simplify the way they are initialized.
🚀 achow101 merged a pull request: "net: Favor peers from addrman over fetching seednodes"
(https://github.com/bitcoin/bitcoin/pull/29605)
(https://github.com/bitcoin/bitcoin/pull/29605)
💬 l0rinc commented on pull request "lint: Check for release note snippets in the wrong folder":
(https://github.com/bitcoin/bitcoin/pull/30812#discussion_r1744155996)
> Seems overkill to import a dependency to check whether a dot is present in a string view or not
Maybe, in that case we can use glob patterns as well with `ls-files`:
```Rust
fn lint_doc_release_note_snippets() -> LintResult {
let result = check_output(git().args(["ls-files", "--", "doc/release-notes/release-notes-*.md", ":!doc/release-notes/*.*.md"]))?;
let snippet_notes = result.lines().collect::<Vec<_>>();
match snippet_notes.as_slice() {
[] => Ok(()),
...
(https://github.com/bitcoin/bitcoin/pull/30812#discussion_r1744155996)
> Seems overkill to import a dependency to check whether a dot is present in a string view or not
Maybe, in that case we can use glob patterns as well with `ls-files`:
```Rust
fn lint_doc_release_note_snippets() -> LintResult {
let result = check_output(git().args(["ls-files", "--", "doc/release-notes/release-notes-*.md", ":!doc/release-notes/*.*.md"]))?;
let snippet_notes = result.lines().collect::<Vec<_>>();
match snippet_notes.as_slice() {
[] => Ok(()),
...
🚀 achow101 merged a pull request: "contrib/signet/miner updates"
(https://github.com/bitcoin/bitcoin/pull/28417)
(https://github.com/bitcoin/bitcoin/pull/28417)
💬 achow101 commented on pull request "test: update satoshi_round function":
(https://github.com/bitcoin/bitcoin/pull/29566#issuecomment-2329615457)
ACK ec317bc44b0cb089419d809b5fef38ecb9423051
(https://github.com/bitcoin/bitcoin/pull/29566#issuecomment-2329615457)
ACK ec317bc44b0cb089419d809b5fef38ecb9423051
🚀 achow101 merged a pull request: "test: update satoshi_round function"
(https://github.com/bitcoin/bitcoin/pull/29566)
(https://github.com/bitcoin/bitcoin/pull/29566)
💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744187608)
Not exactly, this looks more like a trampoline than simple hard-coded examples for just `ArithToUint256` and `UintToArith256`. Both sides depend on the same parameter, which seems circular to me-at least from a black-box perspective. However, I’m fine with this if you think it’s sufficient.
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744187608)
Not exactly, this looks more like a trampoline than simple hard-coded examples for just `ArithToUint256` and `UintToArith256`. Both sides depend on the same parameter, which seems circular to me-at least from a black-box perspective. However, I’m fine with this if you think it’s sufficient.