💬 maflcko commented on pull request "doc: update documentation and scripts related to build directories":
(https://github.com/bitcoin/bitcoin/pull/30741#issuecomment-2325688677)
review ACK 6a68343ffbf3291eb243d90c00df50e672ff3944
(https://github.com/bitcoin/bitcoin/pull/30741#issuecomment-2325688677)
review ACK 6a68343ffbf3291eb243d90c00df50e672ff3944
💬 maflcko commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1741494414)
26c460aa8b5decfd08d931b9b3f80be5c13c7528:
Can you explain this a bit better? Why did it only fail on one CI config, and none of the others? Why was the error an msan runtime-error, as opposed to a link-time error?
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1741494414)
26c460aa8b5decfd08d931b9b3f80be5c13c7528:
Can you explain this a bit better? Why did it only fail on one CI config, and none of the others? Why was the error an msan runtime-error, as opposed to a link-time error?
💬 inyellowbus commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2325791855)
> `bitcoind -reindex` does the trick, and only takes a minute on this tiny chain fortunately.
Reindex doesn't work for me. After starting the node, everything still stacks on block 42335. Maybe I need to do something else before starting the reindex? Or register a connection to some explicit node?
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2325791855)
> `bitcoind -reindex` does the trick, and only takes a minute on this tiny chain fortunately.
Reindex doesn't work for me. After starting the node, everything still stacks on block 42335. Maybe I need to do something else before starting the reindex? Or register a connection to some explicit node?
💬 Sjors commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2325810622)
> everything still stacks on block 42335.
This is excepted. You'll have to wait for a miner to do the same. Welcome to the testnet4 UASF :-)
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2325810622)
> everything still stacks on block 42335.
This is excepted. You'll have to wait for a miner to do the same. Welcome to the testnet4 UASF :-)
💬 hebasto commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1741597310)
> [26c460a](https://github.com/bitcoin/bitcoin/commit/26c460aa8b5decfd08d931b9b3f80be5c13c7528):
>
> Can you explain this a bit better?
I'm afraid I can't (
> Why did it only fail on one CI config, and none of the others? Why was the error an msan runtime-error, as opposed to a link-time error?
Those are the questions I asked myself :)
I'm not able to answer them at this moment. There are two reasons for that:
1. The current set of internal libraries, including `test_fuzz` and `
...
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1741597310)
> [26c460a](https://github.com/bitcoin/bitcoin/commit/26c460aa8b5decfd08d931b9b3f80be5c13c7528):
>
> Can you explain this a bit better?
I'm afraid I can't (
> Why did it only fail on one CI config, and none of the others? Why was the error an msan runtime-error, as opposed to a link-time error?
Those are the questions I asked myself :)
I'm not able to answer them at this moment. There are two reasons for that:
1. The current set of internal libraries, including `test_fuzz` and `
...
💬 josibake commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2325848908)
Concept ACK
Also an implicit approach ACK despite not heavily reviewing the code (yet). I have been focusing on using the kernel library in proof of concept applications to get a better sense of how well the library works for downstream users and to hopefully uncover any pain points preemptively. A few of these projects are linked in the PR description.
Regarding a C header vs C++ header, thanks @ryanofsky for taking the time to explain your thought process. I think you raise some excellen
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2325848908)
Concept ACK
Also an implicit approach ACK despite not heavily reviewing the code (yet). I have been focusing on using the kernel library in proof of concept applications to get a better sense of how well the library works for downstream users and to hopefully uncover any pain points preemptively. A few of these projects are linked in the PR description.
Regarding a C header vs C++ header, thanks @ryanofsky for taking the time to explain your thought process. I think you raise some excellen
...
💬 Sjors commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-2325863585)
Rebased after #30750. It made the diff slightly smaller, since I had made the same changes on every line touched here.
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-2325863585)
Rebased after #30750. It made the diff slightly smaller, since I had made the same changes on every line touched here.
💬 Sjors commented on pull request "scripted-diff: LogPrint -> LogDebug":
(https://github.com/bitcoin/bitcoin/pull/30750#issuecomment-2325864277)
The merge conflicts for #28521 were indeed easy to handle.
(https://github.com/bitcoin/bitcoin/pull/30750#issuecomment-2325864277)
The merge conflicts for #28521 were indeed easy to handle.
👍 hodlinator approved a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2276646525)
ACK a9d19ae1aa06f14c1892bb4e37c3f023bc61b39b
`uint256S()` be gone! :)
Changes as a consequence of removal make sense. Good to see less string parsing testing going on in **arith_uint256_tests.cpp** (covered in recently added `From(User)?Hex`-tests). Arithmetic testing that was going on in **uint256_tests.cpp** has also moved into **arith_uint256_tests.cpp** (`operator_with_self`), could move `conversion`-test too (more in specific comment).
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2276646525)
ACK a9d19ae1aa06f14c1892bb4e37c3f023bc61b39b
`uint256S()` be gone! :)
Changes as a consequence of removal make sense. Good to see less string parsing testing going on in **arith_uint256_tests.cpp** (covered in recently added `From(User)?Hex`-tests). Arithmetic testing that was going on in **uint256_tests.cpp** has also moved into **arith_uint256_tests.cpp** (`operator_with_self`), could move `conversion`-test too (more in specific comment).
💬 hodlinator commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741596200)
nit: I understand this matches the order before the PR but this specific line should probably have the arguments swapped.
```suggestion
BOOST_CHECK_EQUAL(uint256::FromHex(R1ArrayHex).value(), R1L);
```
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741596200)
nit: I understand this matches the order before the PR but this specific line should probably have the arguments swapped.
```suggestion
BOOST_CHECK_EQUAL(uint256::FromHex(R1ArrayHex).value(), R1L);
```
💬 hodlinator commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741633513)
The whole `conversion`-test arguably belongs in the **arith_uint256_tests.cpp** file, which would allow complete removal of `#include <arith_uint256.h>` from this file. That would mirror the code under test, as `uint256` does not depend on `arith_uint256` but the reverse is true.
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741633513)
The whole `conversion`-test arguably belongs in the **arith_uint256_tests.cpp** file, which would allow complete removal of `#include <arith_uint256.h>` from this file. That would mirror the code under test, as `uint256` does not depend on `arith_uint256` but the reverse is true.
💬 hodlinator commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741603274)
Clarify code instead of adding comment?
```suggestion
BOOST_CHECK_EQUAL(uint160::FromHex(std::string_view{R1ArrayHex + 24, 40}).value(), R1S);
```
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741603274)
Clarify code instead of adding comment?
```suggestion
BOOST_CHECK_EQUAL(uint160::FromHex(std::string_view{R1ArrayHex + 24, 40}).value(), R1S);
```
💬 hodlinator commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741581342)
The move of this test to this file and changes to use raw integer literals make total sense.
meganit: If you re-touch - Might just remove this empty line as the comment above applies to the preprocessor logic beneath.
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741581342)
The move of this test to this file and changes to use raw integer literals make total sense.
meganit: If you re-touch - Might just remove this empty line as the comment above applies to the preprocessor logic beneath.
💬 maflcko commented on issue "ci: failure in win64 unit tests":
(https://github.com/bitcoin/bitcoin/issues/30792#issuecomment-2325911157)
Interesting. I re-ran it 12 times on the same config/commit, but it passed every time: https://cirrus-ci.com/task/6025886804213760
It would be nice if the test failure was a bit more verbose to show the full traceback, as well as all values involved.
(https://github.com/bitcoin/bitcoin/issues/30792#issuecomment-2325911157)
Interesting. I re-ran it 12 times on the same config/commit, but it passed every time: https://cirrus-ci.com/task/6025886804213760
It would be nice if the test failure was a bit more verbose to show the full traceback, as well as all values involved.
💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-2325914293)
`f5fe172e85...127c86d91f`: pet clang-tidy: use `= default` instead of `{}`
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-2325914293)
`f5fe172e85...127c86d91f`: pet clang-tidy: use `= default` instead of `{}`
💬 Sjors commented on pull request "Add Signet launch shortcut for Windows":
(https://github.com/bitcoin/bitcoin/pull/26334#issuecomment-2325921328)
Rebased for CMake (dropping `src/Makefile.qt.include`) and added a testnet4 shortcut.
(https://github.com/bitcoin/bitcoin/pull/26334#issuecomment-2325921328)
Rebased for CMake (dropping `src/Makefile.qt.include`) and added a testnet4 shortcut.
💬 maflcko commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1741659568)
Ok, so I guess the reason is that this is required to mirror the autotools approach from https://github.com/bitcoin/bitcoin/blob/5abb9b1af49be9024f95fa2f777285c531785d85/src/Makefile.test.include#L44 ?
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1741659568)
Ok, so I guess the reason is that this is required to mirror the autotools approach from https://github.com/bitcoin/bitcoin/blob/5abb9b1af49be9024f95fa2f777285c531785d85/src/Makefile.test.include#L44 ?
💬 fanquake commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#issuecomment-2325944033)
> Sets the "Debug" build configuration when depends are built with DEBUG=1, ensuring that linux_debug_CPPFLAGS from depends are passed to the main build system.
Is this not a bug? If depends is built (in any configuration/with any options/flags) then the expectation would be that everything is passed to CMake (regardless of the CMake build type). However it seems like your saying CMake will actually just ignore some flags depending on which build type has been set? If this was the intended be
...
(https://github.com/bitcoin/bitcoin/pull/30778#issuecomment-2325944033)
> Sets the "Debug" build configuration when depends are built with DEBUG=1, ensuring that linux_debug_CPPFLAGS from depends are passed to the main build system.
Is this not a bug? If depends is built (in any configuration/with any options/flags) then the expectation would be that everything is passed to CMake (regardless of the CMake build type). However it seems like your saying CMake will actually just ignore some flags depending on which build type has been set? If this was the intended be
...
💬 hebasto commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1741675137)
Autotools tends to overlink, while in CMake, I aimed to link only the necessary components.
Yes, this PR commit mirrors the line from `bitcoin/src/Makefile.test.include` that you mentioned. However, simply removing that line from 80f00cafdeef0600fa1a5e906686720786c2336c results in a link-time error.
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1741675137)
Autotools tends to overlink, while in CMake, I aimed to link only the necessary components.
Yes, this PR commit mirrors the line from `bitcoin/src/Makefile.test.include` that you mentioned. However, simply removing that line from 80f00cafdeef0600fa1a5e906686720786c2336c results in a link-time error.
💬 hodlinator commented on pull request "refactor: Extend CScript with `operator<<` for `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741676894)
Having an elegant way of expressing scripts in code is nice. How about (omitted some hex digits):
```C++
script << "046711d5f"_sized_hex << OP_CHECKSIG;
```
`_sized_hex` could possibly create something like a `SizePrefixed(std::array<std::byte>)` object and `CScript` could have a `<<` operator taking such objects and prepending the size.
> But I do think it would be nice if this PR just added support for std::span and std::byte without necessarily making bigger changes.
That was what I
...
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741676894)
Having an elegant way of expressing scripts in code is nice. How about (omitted some hex digits):
```C++
script << "046711d5f"_sized_hex << OP_CHECKSIG;
```
`_sized_hex` could possibly create something like a `SizePrefixed(std::array<std::byte>)` object and `CScript` could have a `<<` operator taking such objects and prepending the size.
> But I do think it would be nice if this PR just added support for std::span and std::byte without necessarily making bigger changes.
That was what I
...
💬 stratospher commented on pull request "rpc: Add test-only RPCs under `-test=<option>` flag":
(https://github.com/bitcoin/bitcoin/pull/30717#discussion_r1741679713)
makes sense. moving regtest only options to `-test=<opt>` would make the interface(dev-testing/more-debug-info vs regtest-only) less confusing. (other than test=addrman), regtest-only would include: `fastprune`, `mocktime`, `acceptstalefeeestimates`.
I think `peertimeout` and `rpcdoccheck` are only used in tests, so you could include them too
(https://github.com/bitcoin/bitcoin/pull/30717#discussion_r1741679713)
makes sense. moving regtest only options to `-test=<opt>` would make the interface(dev-testing/more-debug-info vs regtest-only) less confusing. (other than test=addrman), regtest-only would include: `fastprune`, `mocktime`, `acceptstalefeeestimates`.
I think `peertimeout` and `rpcdoccheck` are only used in tests, so you could include them too