Bitcoin Core Github
44 subscribers
120K links
Download Telegram
🚀 ryanofsky merged a pull request: "test: add mocked Sock that can read/write custom data and/or CNetMessages"
(https://github.com/bitcoin/bitcoin/pull/30205)
💬 ryanofsky commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2648053825)
Merged this, and one thing I was curious about looking at the code was if it makes sense to replace `void* buf, size_t len` which is used many places with `std::span<std::byte>` here? That seems like could make it safer or easier to use
💬 ryanofsky commented on pull request "build: disable bitcoin-node if daemon is not built":
(https://github.com/bitcoin/bitcoin/pull/31834#issuecomment-2648071438)
> But [7e11ee6](https://github.com/bitcoin/bitcoin/commit/7e11ee6ad9b04546b900e38dee349db821415ba7) modifies only the summary output.

Maybe also should include:

```diff
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 89fdd855a459..8c42359d2d55 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -320,7 +320,7 @@ if(BUILD_DAEMON)
)
list(APPEND installable_targets bitcoind)
endif()
-if(WITH_MULTIPROCESS)
+if(WITH_MULTIPROCESS AND BUILD_DAEMON)
add_execu
...
🤔 Eunovo reviewed a pull request: "wallet: abandon orphan coinbase txs, and their descendants, during startup"
(https://github.com/bitcoin/bitcoin/pull/31794#pullrequestreview-2605174936)
ACK https://github.com/bitcoin/bitcoin/pull/31794/commits/0331060d1354bc8620564e6352946bd5be38fef3

While testing, I noticed that trying to abandon a tx in mempool wasn't covered in the functional tests.
Here's a diff that fixes that.
```diff
diff --git a/test/functional/wallet_abandonconflict.py b/test/functional/wallet_abandonconflict.py
index ce0f4d099b..2bbfaee3db 100755
--- a/test/functional/wallet_abandonconflict.py
+++ b/test/functional/wallet_abandonconflict.py
@@ -45,6 +45,1
...
💬 Eunovo commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1948702897)
https://github.com/bitcoin/bitcoin/pull/31794/commits/0331060d1354bc8620564e6352946bd5be38fef3: `# Now stop both nodes...`
💬 Sjors commented on pull request "build: disable bitcoin-node if daemon is not built":
(https://github.com/bitcoin/bitcoin/pull/31834#issuecomment-2648127639)
Added, and updated the PR description.
💬 Abdiselammuktar commented on pull request "build: disable bitcoin-node if daemon is not built":
(https://github.com/bitcoin/bitcoin/pull/31834#issuecomment-2648151854)
31834

On Mon, 10 Feb 2025, 5:22 pm Sjors Provoost, ***@***.***>
wrote:

> Added, and updated the PR description.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/pull/31834#issuecomment-2648127639>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/A75ANGU23VFAZ4MZ4C35CUT2PCY3HAVCNFSM6AAAAABW2SSDQ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNBYGEZDONRTHE>
> .
> You are receiving this because you are subscribed to this
...
🤔 hodlinator reviewed a pull request: "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds"
(https://github.com/bitcoin/bitcoin/pull/31588#pullrequestreview-2605954001)
Concept ACK 459683bcfc70b77b3dae2e70c88cf5c6a007442e

Changes are certainly a step in the right direction unless we decide to delete the script.

### Tested on NixOS

Without PR, it does indeed fail:
```
₿ ./contrib/devtools/test_deterministic_coverage.sh
Error: Executable src/test/test_bitcoin not found. Run "cmake -B build -DCMAKE_BUILD_TYPE=Coverage" and compile.
```

With the PR I'm getting a failure inside gcov - `AssertionError: count must not be a negative value.`.

<details
...
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2648185796)
Rebased after #31384.
🤔 hebasto reviewed a pull request: "build: disable bitcoin-node if daemon is not built"
(https://github.com/bitcoin/bitcoin/pull/31834#pullrequestreview-2606025160)
Concept ACK.

On the master branch:
```
$ gmake -C depends NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_USDT=1 MULTIPROCESS=1
$ cmake -B build -G Ninja --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake -DBUILD_FOR_FUZZING=ON
$ ninja -C build -t targets | grep -E "bitcoind|bitcoin-node"
bitcoin-node: phony
```

With this PR, the last command's output is empty.
👍 hebasto approved a pull request: "build: disable bitcoin-node if daemon is not built"
(https://github.com/bitcoin/bitcoin/pull/31834#pullrequestreview-2606067407)
ACK 2ffea09820e66e25ab639c9fc14810fd96ad6213.
🚀 fanquake merged a pull request: "doc: swap CPPFLAGS for APPEND_CPPFLAGS"
(https://github.com/bitcoin/bitcoin/pull/31819)
📝 stratospher opened a pull request: "validation: set BLOCK_FAILED_CHILD correctly"
(https://github.com/bitcoin/bitcoin/pull/31835)
This PR addresses 2 issues related to how `BLOCK_FAILED_CHILD` is set:
1. In `InvalidateBlock()`
- Previously, `BLOCK_FAILED_CHILD` was not being set when it should have been.
- This was due to an incorrect traversal condition, which is fixed in this PR.

2. In `SetBlockFailure()`
- `BLOCK_FAILED_VALID` is now cleared before setting `BLOCK_FAILED_CHILD.`

Also adds a unit test to check `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` status in `InvalidateBlock()`.


#### looking for feed
...
👍 ryanofsky approved a pull request: "build: disable bitcoin-node if daemon is not built"
(https://github.com/bitcoin/bitcoin/pull/31834#pullrequestreview-2606125408)
Code review ACK 2ffea09820e66e25ab639c9fc14810fd96ad6213
💬 maflcko commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2648315209)
Given all the GCC/lcov/gcov problems, I wonder if it wouldn't be easier to just go with clang/llvm:

* Require compilation with `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DCMAKE_CXX_FLAGS='-fPIC -fprofile-instr-generate -fcoverage-mapping'`
* apply the below diff

```diff
diff --git a/contrib/devtools/test_deterministic_coverage.sh b/contrib/devtools/test_deterministic_coverage.sh
index 885396bb25..48d331ae70 100755
--- a/contrib/devtools/test_deterministic_coverage.sh

...
🤔 stratospher reviewed a pull request: "validation: stricter internal handling of invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2598002988)
reACK 4ba2e48.
💬 stratospher commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r1944318554)
eca8d91: micro-style-nit only if you have to retouch - could make double (()) to ().
💬 hebasto commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2648416127)
> Attempted to run Guix build [e717fb5](https://github.com/bitcoin/bitcoin/commit/e717fb5a429d9d00e008fa1eb2b85179050be8fe) cross-built for Windows, but it fails already on test "0" / line 133:

Is this failure introduced by this PR, or does it occur on the master branch as well?

> Was able to find a solution on [StackOverflow](https://stackoverflow.com/questions/69560731) - activated _Developer Mode_ in Windows settings to avoid it.

There are many [ways](https://neacsu.net/posts/win_sym
...
🤔 ryanofsky reviewed a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2606185530)
Rebased e7743aa5df6387964c23f0629debf8c3cbb8ed4c -> e484d0bd11408489d83aaecf49a238f98a117696 ([`pr/subtree.13`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.13) -> [`pr/subtree.14`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.14), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/subtree.13-rebase..pr/subtree.14)) adding https://github.com/chaincodelabs/libmultiprocess/pull/161 and trying another suggested readability improvement to depends code, hiding more capnp
...