💬 fanquake commented on issue "test: intermittent issue in p2p_1p1c_network.py":
(https://github.com/bitcoin/bitcoin/issues/33318#issuecomment-3503644320)
Seen again in https://github.com/bitcoin/bitcoin/actions/runs/19173508101/job/54812101221?pr=33810.
(https://github.com/bitcoin/bitcoin/issues/33318#issuecomment-3503644320)
Seen again in https://github.com/bitcoin/bitcoin/actions/runs/19173508101/job/54812101221?pr=33810.
💬 151henry151 commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3503646384)
> Sorry, but one more rebase is needed.
I think I've rebased correctly here -- I removed an additional `SKIP_BUILD_RPATH OFF` override that seemed like it would no longer be needed, https://github.com/bitcoin/bitcoin/commit/a02282d20c87471a3399e4061c7edad7ecdb391f#diff-2a07660db8ebdbbf967cf42f6b4da3ffc8558a322f45a3a08ce292c9e91767b8L13
Guix build for a02282d20c87:
a3647a46e00124b207263b695d9598cf9fe6c845ea9072c933462dd0e31a0ed9 guix-build-a02282d20c87/output/aarch64-linux-gnu/SHA256SUMS
...
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3503646384)
> Sorry, but one more rebase is needed.
I think I've rebased correctly here -- I removed an additional `SKIP_BUILD_RPATH OFF` override that seemed like it would no longer be needed, https://github.com/bitcoin/bitcoin/commit/a02282d20c87471a3399e4061c7edad7ecdb391f#diff-2a07660db8ebdbbf967cf42f6b4da3ffc8558a322f45a3a08ce292c9e91767b8L13
Guix build for a02282d20c87:
a3647a46e00124b207263b695d9598cf9fe6c845ea9072c933462dd0e31a0ed9 guix-build-a02282d20c87/output/aarch64-linux-gnu/SHA256SUMS
...
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3503650032)
It's ready for testing with:
- https://github.com/stratum-mining/sv2-tp/pull/59
I also pushed a small change that switches `ExtractCoinbaseTemplate` to take `CTransactionRef coinbase_tx` as its argument rather than a `CBlockTemplate`. It doesn't really make a difference here, but lets me copy-paste it to `sv2-tp` which doesn't have `CBlockTemplate`.
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3503650032)
It's ready for testing with:
- https://github.com/stratum-mining/sv2-tp/pull/59
I also pushed a small change that switches `ExtractCoinbaseTemplate` to take `CTransactionRef coinbase_tx` as its argument rather than a `CBlockTemplate`. It doesn't really make a difference here, but lets me copy-paste it to `sv2-tp` which doesn't have `CBlockTemplate`.
💬 fanquake commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3503682477)
The TSAN failure is #33318.
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3503682477)
The TSAN failure is #33318.
📝 fanquake opened a pull request: "guix: build glibc with `--enable-static-pie`"
(https://github.com/bitcoin/bitcoin/pull/33821)
From the glibc `2.27` release notes:
> Major new features:
> The GNU C Library can now be compiled with support for building static
> PIE executables (See --enable-static-pie in INSTALL).
> These static PIE executables are like static executables but can be
> loaded at any address and provide additional security hardening benefits
> at the cost of some memory and performance. When the library is built with
> --enable-static-pic the resulting libc.a is usable with GCC 8 and above to
>
...
(https://github.com/bitcoin/bitcoin/pull/33821)
From the glibc `2.27` release notes:
> Major new features:
> The GNU C Library can now be compiled with support for building static
> PIE executables (See --enable-static-pie in INSTALL).
> These static PIE executables are like static executables but can be
> loaded at any address and provide additional security hardening benefits
> at the cost of some memory and performance. When the library is built with
> --enable-static-pic the resulting libc.a is usable with GCC 8 and above to
>
...
💬 l0rinc commented on pull request "test: Enhance GetTxSigOpCost tests for coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/32840#issuecomment-3503719786)
Thanks @average-gary.
Would it be possible to group the commits so that they're telling a story, so that the commit messages explain the "why", not the "what". The "what" is already in the code. The commit messages seem LLM generated, they don't add a lot of value this way.
The first commit still adds a test that claims we're testing the witness when in fact it's empty: [`a4376ab` (#32840)](https://github.com/bitcoin/bitcoin/pull/32840/commits/a4376abd72aae5c0ea158db66440618cb1abfe5a#r24921793
...
(https://github.com/bitcoin/bitcoin/pull/32840#issuecomment-3503719786)
Thanks @average-gary.
Would it be possible to group the commits so that they're telling a story, so that the commit messages explain the "why", not the "what". The "what" is already in the code. The commit messages seem LLM generated, they don't add a lot of value this way.
The first commit still adds a test that claims we're testing the witness when in fact it's empty: [`a4376ab` (#32840)](https://github.com/bitcoin/bitcoin/pull/32840/commits/a4376abd72aae5c0ea158db66440618cb1abfe5a#r24921793
...
💬 purpleKarrot commented on pull request "cmake: Create subdirectories in build tree in advance":
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3503730840)
> I think there is no explanation in the code, but the reason why the copies/symlinks are created is that it keeps the source dir clean of `.pyc` files.
The generation of `.pyc` files can be suppressed with python's `-B` option or `PYTHONDONTWRITEBYTECODE` env var.
See: https://docs.python.org/3/using/cmdline.html#envvar-PYTHONDONTWRITEBYTECODE
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3503730840)
> I think there is no explanation in the code, but the reason why the copies/symlinks are created is that it keeps the source dir clean of `.pyc` files.
The generation of `.pyc` files can be suppressed with python's `-B` option or `PYTHONDONTWRITEBYTECODE` env var.
See: https://docs.python.org/3/using/cmdline.html#envvar-PYTHONDONTWRITEBYTECODE
💬 Sjors commented on issue "Header-only support for waitNext()":
(https://github.com/bitcoin/bitcoin/issues/33756#issuecomment-3503736237)
>> I'd like to expand the `waitNext()` Mining IPC function to optionally (via BlockWaitOptions) return an (empty) new template as soon as we have a header with sufficient proof-of-work, pending block download and/or validation.
> Why? you should expand on the motivations behind this and what exactly you aim to achieve with this feature.
To allow miners to start hashing on an empty block template as soon as we see a new header, rather than waiting for the block to be fully downloaded and valida
...
(https://github.com/bitcoin/bitcoin/issues/33756#issuecomment-3503736237)
>> I'd like to expand the `waitNext()` Mining IPC function to optionally (via BlockWaitOptions) return an (empty) new template as soon as we have a header with sufficient proof-of-work, pending block download and/or validation.
> Why? you should expand on the motivations behind this and what exactly you aim to achieve with this feature.
To allow miners to start hashing on an empty block template as soon as we see a new header, rather than waiting for the block to be fully downloaded and valida
...
💬 ismaelsadeeq commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504658204)
> In the longer run it probably makes sense for sv2-tp to have one major release of sv2-tp for every major Bitcoin Core release, but current it doesn't have backports. Each release is a combination of bug fixes and new features.
Hmm, fwiw I am approach NACK changes that introduce conflicting interface method that deprecate and don't prune on experimental interface that did not offer interface stability for the reason discussed above.
However I am open minded to convincing otherwise and cur
...
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504658204)
> In the longer run it probably makes sense for sv2-tp to have one major release of sv2-tp for every major Bitcoin Core release, but current it doesn't have backports. Each release is a combination of bug fixes and new features.
Hmm, fwiw I am approach NACK changes that introduce conflicting interface method that deprecate and don't prune on experimental interface that did not offer interface stability for the reason discussed above.
However I am open minded to convincing otherwise and cur
...
📝 yuvicc opened a pull request: "[kernel] Add block header support and validation"
(https://github.com/bitcoin/bitcoin/pull/33822)
Adds a new `btck_BlockHeader` type and associated functions to create, access, and validate block headers. Block headers will have their own type (`btck_BlockHeader`) that can be created from raw data, copied, and queried for all the standard header fields (hash, prev hash, timestamp, bits, version, nonce). We can also extract headers from full blocks or block tree entries.
#### New Block Header API
- **`btck_BlockHeader` type**: Opaque handle for block headers
- **Header methods**:
...
(https://github.com/bitcoin/bitcoin/pull/33822)
Adds a new `btck_BlockHeader` type and associated functions to create, access, and validate block headers. Block headers will have their own type (`btck_BlockHeader`) that can be created from raw data, copied, and queried for all the standard header fields (hash, prev hash, timestamp, bits, version, nonce). We can also extract headers from full blocks or block tree entries.
#### New Block Header API
- **`btck_BlockHeader` type**: Opaque handle for block headers
- **Header methods**:
...
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504684591)
The old methods are redundant, but they're not conflicting. Clients can use either and both have test coverage.
From this projects perspective there's hardly any additional review burden when we prune deprecated methods in a later PR, that should be trivial.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2504684591)
The old methods are redundant, but they're not conflicting. Clients can use either and both have test coverage.
From this projects perspective there's hardly any additional review burden when we prune deprecated methods in a later PR, that should be trivial.
💬 maflcko commented on pull request "cmake: Create subdirectories in build tree in advance":
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3503780491)
I know, but `-B` does not help to fix the other problems: (1) They are not run via ctest, and (2) they are stand-alone scripts, where the user/dev would have to specify it every time, or set the env var.
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3503780491)
I know, but `-B` does not help to fix the other problems: (1) They are not run via ctest, and (2) they are stand-alone scripts, where the user/dev would have to specify it every time, or set the env var.
💬 fanquake commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#issuecomment-3503803553)
https://github.com/bitcoin/bitcoin/actions/runs/19175802541/job/54820162776?pr=33822#step:5:179:
```bash
<snip>
src/kernel/bitcoinkernel.h:1726: *
src/kernel/bitcoinkernel_wrapper.h:759:
src/kernel/bitcoinkernel_wrapper.h:762:
^^^
Trailing whitespace (including Windows line endings [CR LF]) is problematic
```
(https://github.com/bitcoin/bitcoin/pull/33822#issuecomment-3503803553)
https://github.com/bitcoin/bitcoin/actions/runs/19175802541/job/54820162776?pr=33822#step:5:179:
```bash
<snip>
src/kernel/bitcoinkernel.h:1726: *
src/kernel/bitcoinkernel_wrapper.h:759:
src/kernel/bitcoinkernel_wrapper.h:762:
^^^
Trailing whitespace (including Windows line endings [CR LF]) is problematic
```
💬 hebasto commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#discussion_r2504727853)
The `CMAKE_<TYPE>_LINKER_FLAGS` variables are quite invasive. They override any initial values set either in a toolchain file via `CMAKE_<TYPE>_LINKER_FLAGS_INIT` or in the `LDFLAGS` environment variable.
Fortunately, there are no linker flags set for Linux hosts in depends.
Still, I suggest documenting this behaviour for our future selves.
(https://github.com/bitcoin/bitcoin/pull/33181#discussion_r2504727853)
The `CMAKE_<TYPE>_LINKER_FLAGS` variables are quite invasive. They override any initial values set either in a toolchain file via `CMAKE_<TYPE>_LINKER_FLAGS_INIT` or in the `LDFLAGS` environment variable.
Fortunately, there are no linker flags set for Linux hosts in depends.
Still, I suggest documenting this behaviour for our future selves.
💬 fanquake commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#discussion_r2504749571)
Ok, given the behaviour here, we can add some documentation if we end up taking this approach in future.
(https://github.com/bitcoin/bitcoin/pull/33181#discussion_r2504749571)
Ok, given the behaviour here, we can add some documentation if we end up taking this approach in future.
💬 maflcko commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2504767099)
Maybe:
```sh
echo "^^^ ⚠️ Failure generated from IWYU"
echo
echo "Some adjustments to the diff may be needed:"
echo
echo "* Use the C++ headers: E.g. <cerrno> over <errno.h>"
echo '* Use #include <__.h> over #include "__.h" for all headers'
echo "* Sort the headers, so that they are in the right pre-existing section, if"
echo " there are any."
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2504767099)
Maybe:
```sh
echo "^^^ ⚠️ Failure generated from IWYU"
echo
echo "Some adjustments to the diff may be needed:"
echo
echo "* Use the C++ headers: E.g. <cerrno> over <errno.h>"
echo '* Use #include <__.h> over #include "__.h" for all headers'
echo "* Sort the headers, so that they are in the right pre-existing section, if"
echo " there are any."
🤔 hebasto reviewed a pull request: "guix: build for Linux HOSTS with `-static-libgcc`"
(https://github.com/bitcoin/bitcoin/pull/33181#pullrequestreview-3435466193)
I have reviewed the code and it looks OK.
Still waiting for my Guix build on RISC-V machine.
(https://github.com/bitcoin/bitcoin/pull/33181#pullrequestreview-3435466193)
I have reviewed the code and it looks OK.
Still waiting for my Guix build on RISC-V machine.
💬 hebasto commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2504805636)
Thanks! Added.
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2504805636)
Thanks! Added.
💬 marcofleon commented on issue "RFC: Add multiprocess fuzz target":
(https://github.com/bitcoin/bitcoin/issues/23015#issuecomment-3503990755)
> dreading the crashes
While we are looking forward to them :)
> Are there changes that would make it more maintainable?
@dergoegge could speak more to this, but an initial step would probably be to enable fuzzamoto to more easily build the multiprocess binaries. Not having to manually change Dockerfiles and `Cargo.toml`. And then also, we could have the various clients separated out from the scenarios themselves (or one general IPC client that wraps the others maybe).
Then it would just be
...
(https://github.com/bitcoin/bitcoin/issues/23015#issuecomment-3503990755)
> dreading the crashes
While we are looking forward to them :)
> Are there changes that would make it more maintainable?
@dergoegge could speak more to this, but an initial step would probably be to enable fuzzamoto to more easily build the multiprocess binaries. Not having to manually change Dockerfiles and `Cargo.toml`. And then also, we could have the various clients separated out from the scenarios themselves (or one general IPC client that wraps the others maybe).
Then it would just be
...
📝 portlandhodl converted_to_draft a pull request: "Enhanced error messages for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260)
### **Issue:**
Simply `DecodeDestination` does not handle errors where the user inputs a valid address for the wrong network. _e.x. testnet while running client on mainnet_
The current error `not a valid Bech32 or Base58 encoding` for a valid address on a different network is entirely incorrect. This is because of the is_bech32 variable used at the core of `DecodeDestination` logic only checks that the prefix matches. If it doesn't it just starts running everything as `DecodeBase58Check
...
(https://github.com/bitcoin/bitcoin/pull/27260)
### **Issue:**
Simply `DecodeDestination` does not handle errors where the user inputs a valid address for the wrong network. _e.x. testnet while running client on mainnet_
The current error `not a valid Bech32 or Base58 encoding` for a valid address on a different network is entirely incorrect. This is because of the is_bech32 variable used at the core of `DecodeDestination` logic only checks that the prefix matches. If it doesn't it just starts running everything as `DecodeBase58Check
...