🚀 fanquake merged a pull request: "wallet: don't consider unconfirmed TRUC coins with ancestors"
(https://github.com/bitcoin/bitcoin/pull/33528)
(https://github.com/bitcoin/bitcoin/pull/33528)
📝 rustaceanrob opened a pull request: "Implementation of SwiftSync"
(https://github.com/bitcoin/bitcoin/pull/34004)
PR description in progress.
Link to the protocol, terminology, and writeup found [here](https://gist.github.com/RubenSomsen/a61a37d14182ccd78760e477c78133cd).
(https://github.com/bitcoin/bitcoin/pull/34004)
PR description in progress.
Link to the protocol, terminology, and writeup found [here](https://gist.github.com/RubenSomsen/a61a37d14182ccd78760e477c78133cd).
💬 fanquake commented on pull request "wallet: don't consider unconfirmed TRUC coins with ancestors":
(https://github.com/bitcoin/bitcoin/pull/33528#issuecomment-3611250084)
Backported to `30.x` in #33997.
(https://github.com/bitcoin/bitcoin/pull/33528#issuecomment-3611250084)
Backported to `30.x` in #33997.
💬 fanquake commented on pull request "test: interface_ipc.py minor fixes and cleanup":
(https://github.com/bitcoin/bitcoin/pull/34003#issuecomment-3611379248)
cc @Sjors
(https://github.com/bitcoin/bitcoin/pull/34003#issuecomment-3611379248)
cc @Sjors
🤔 janb84 reviewed a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3539275351)
re ACK d9319b06cf82664d55f255387a348135fd7f91c7
``` C++
bool PeerManagerImpl::IsBlockRequested(const uint256& hash)
{
return mapBlocksInFlight.count(hash);
}
```
"Ah returns a count => int , oh no it's bool"
VS
```C++
bool PeerManagerImpl::IsBlockRequested(const uint256& hash)
{
return mapBlocksInFlight.contains(hash);
}
```
"ah does it contain X => yes/no "
Clearly the intent is better conveyed. I'm still a big proponent of this change.
(https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3539275351)
re ACK d9319b06cf82664d55f255387a348135fd7f91c7
``` C++
bool PeerManagerImpl::IsBlockRequested(const uint256& hash)
{
return mapBlocksInFlight.count(hash);
}
```
"Ah returns a count => int , oh no it's bool"
VS
```C++
bool PeerManagerImpl::IsBlockRequested(const uint256& hash)
{
return mapBlocksInFlight.contains(hash);
}
```
"ah does it contain X => yes/no "
Clearly the intent is better conveyed. I'm still a big proponent of this change.
💬 Sjors commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3611468373)
> Just not returning new templates after a certain amount of memory has been used would like a simpler approach.
It is, but refusing to make new templates doesn't stop the footprint of existing templates from growing. The worst case extra memory footprint for _existing_ templates is the full size of the mempool.
This is rather unlikely though, it would only happen if between two blocks the entire mempool was gradually RBF'd in such a way that each transaction was at the top of the mempool
...
(https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3611468373)
> Just not returning new templates after a certain amount of memory has been used would like a simpler approach.
It is, but refusing to make new templates doesn't stop the footprint of existing templates from growing. The worst case extra memory footprint for _existing_ templates is the full size of the mempool.
This is rather unlikely though, it would only happen if between two blocks the entire mempool was gradually RBF'd in such a way that each transaction was at the top of the mempool
...
💬 Sjors commented on pull request "test: interface_ipc.py minor fixes and cleanup":
(https://github.com/bitcoin/bitcoin/pull/34003#issuecomment-3611494379)
> Destroy calls were being made at the end of the test instead of after templates were no longer needed.
I take advantage of that in #33922, but I'll figure out a way to rebase if needed. E.g. I can just add another test with multiple templates.
I tested on macOS 26.1 that the tests still pass.
Can you split this into a few commits as it's quite a long list of changes now.
(https://github.com/bitcoin/bitcoin/pull/34003#issuecomment-3611494379)
> Destroy calls were being made at the end of the test instead of after templates were no longer needed.
I take advantage of that in #33922, but I'll figure out a way to rebase if needed. E.g. I can just add another test with multiple templates.
I tested on macOS 26.1 that the tests still pass.
Can you split this into a few commits as it's quite a long list of changes now.
👍 stickies-v approved a pull request: "init: point out -stopatheight may be imprecise"
(https://github.com/bitcoin/bitcoin/pull/33993#pullrequestreview-3539444459)
ACK ff06e2468a5d3eeebeffe781904c34c9d1b44385
(https://github.com/bitcoin/bitcoin/pull/33993#pullrequestreview-3539444459)
ACK ff06e2468a5d3eeebeffe781904c34c9d1b44385
💬 hebasto commented on issue "Consider enabling plugin=wayland for bitcoincore.org builds":
(https://github.com/bitcoin-core/gui/issues/916#issuecomment-3611707501)
> I think it's safe to enable `plugin=wayland` by default for builds on bitcoincore.org.
A discussion on this topic took place in https://github.com/bitcoin/bitcoin/pull/22708.
The main concern raised was the introduction of additional dependencies: https://github.com/bitcoin/bitcoin/pull/22708#issuecomment-1100861599).
(https://github.com/bitcoin-core/gui/issues/916#issuecomment-3611707501)
> I think it's safe to enable `plugin=wayland` by default for builds on bitcoincore.org.
A discussion on this topic took place in https://github.com/bitcoin/bitcoin/pull/22708.
The main concern raised was the introduction of additional dependencies: https://github.com/bitcoin/bitcoin/pull/22708#issuecomment-1100861599).
💬 hebasto commented on pull request "build, qt: Add Wayland support for Linux builds with depends":
(https://github.com/bitcoin/bitcoin/pull/22708#issuecomment-3611709330)
Also: https://github.com/bitcoin-core/gui/issues/916.
(https://github.com/bitcoin/bitcoin/pull/22708#issuecomment-3611709330)
Also: https://github.com/bitcoin-core/gui/issues/916.
💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3611774702)
@maflcko suggested just declaring the util functions. This has reduced test.cpp further, with the same non-determinism (https://github.com/fanquake/bitcoin/commit/c12ef2a5312cb66cd01a2f1230fe4e976759f4e1).
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3611774702)
@maflcko suggested just declaring the util functions. This has reduced test.cpp further, with the same non-determinism (https://github.com/fanquake/bitcoin/commit/c12ef2a5312cb66cd01a2f1230fe4e976759f4e1).
💬 sedited commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2588768798)
Looking at the additional branches introduced here by these swiftsync conditionals, I wonder if a better approach would be having a dedicated swiftsync coins view, that takes care of swiftsync specific operations without adding extra logic to the actual validation code. Not sure what the best way would be to do this, but I guess it could look similarly to the async coins view introduced in #31132. Then again, the current caching structure is already complicated enough and moving the complexity o
...
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2588768798)
Looking at the additional branches introduced here by these swiftsync conditionals, I wonder if a better approach would be having a dedicated swiftsync coins view, that takes care of swiftsync specific operations without adding extra logic to the actual validation code. Not sure what the best way would be to do this, but I guess it could look similarly to the async coins view introduced in #31132. Then again, the current caching structure is already complicated enough and moving the complexity o
...
💬 hebasto commented on pull request "doc: Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#issuecomment-3611961731)
> Previously one had to read the Makefile (and various *.mk configuration
> files) to see how to correctly override CC and CXX when building native
> depends packages.
I also wanted to highlight the distinction between environment variables and Makefile variables. There should be no difference in behaviour between the two when supplied by the user, provided they are recognised by the build system. But that is not currently the case. See: https://github.com/bitcoin/bitcoin/pull/29963.
(https://github.com/bitcoin/bitcoin/pull/33902#issuecomment-3611961731)
> Previously one had to read the Makefile (and various *.mk configuration
> files) to see how to correctly override CC and CXX when building native
> depends packages.
I also wanted to highlight the distinction between environment variables and Makefile variables. There should be no difference in behaviour between the two when supplied by the user, provided they are recognised by the build system. But that is not currently the case. See: https://github.com/bitcoin/bitcoin/pull/29963.
👍 pablomartin4btc approved a pull request: "init: point out -stopatheight may be imprecise"
(https://github.com/bitcoin/bitcoin/pull/33993#pullrequestreview-3539838956)
ACK ff06e2468a5d3eeebeffe781904c34c9d1b44385
(https://github.com/bitcoin/bitcoin/pull/33993#pullrequestreview-3539838956)
ACK ff06e2468a5d3eeebeffe781904c34c9d1b44385
💬 rustaceanrob commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2588961718)
To suppress the UB sanitizer here I added a `__attribute__(no_sanitize ... ))` which cause a CI failure on windows. Is there 1. a wrapping addition/subtraction API I am not aware of 2. a way to suppress UB across all targets
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2588961718)
To suppress the UB sanitizer here I added a `__attribute__(no_sanitize ... ))` which cause a CI failure on windows. Is there 1. a wrapping addition/subtraction API I am not aware of 2. a way to suppress UB across all targets
💬 Sjors commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2588964479)
Ok, done and I rebased #33966 on that change.
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2588964479)
Ok, done and I rebased #33966 on that change.
💬 hebasto commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#discussion_r2589020763)
> But I don't understand why the `#include <src/...>` lines were working previously?
This code is responsible for adding the required include path:https://github.com/bitcoin/bitcoin/blob/75baff98fcf987735437196a4db1919e390c4bd2/src/ipc/libmultiprocess/cmake/TargetCapnpSources.cmake#L96-L103
(https://github.com/bitcoin/bitcoin/pull/33774#discussion_r2589020763)
> But I don't understand why the `#include <src/...>` lines were working previously?
This code is responsible for adding the required include path:https://github.com/bitcoin/bitcoin/blob/75baff98fcf987735437196a4db1919e390c4bd2/src/ipc/libmultiprocess/cmake/TargetCapnpSources.cmake#L96-L103
💬 Sjors commented on pull request "refactor: disentangle miner startup defaults from runtime options":
(https://github.com/bitcoin/bitcoin/pull/33966#issuecomment-3612267713)
I moved `MiningArgs` and static `DEFAULT_PRINT_MODIFIED_FEE` from `node/mining.h` to `node/mining_args.h`.
> You mean you want the mining interface options separate from the `TransactionError` `TxBroadcast` enum types?
Yes, I think it's easier for Mining IPC client developers to understand how things work that way.
(https://github.com/bitcoin/bitcoin/pull/33966#issuecomment-3612267713)
I moved `MiningArgs` and static `DEFAULT_PRINT_MODIFIED_FEE` from `node/mining.h` to `node/mining_args.h`.
> You mean you want the mining interface options separate from the `TransactionError` `TxBroadcast` enum types?
Yes, I think it's easier for Mining IPC client developers to understand how things work that way.
👍 sedited approved a pull request: "merkle: migrate `path` arg to reference and drop unused args"
(https://github.com/bitcoin/bitcoin/pull/33805#pullrequestreview-3540024475)
ACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94
(https://github.com/bitcoin/bitcoin/pull/33805#pullrequestreview-3540024475)
ACK 24ed820d4f0d8f7fa2f69e1909c2d98f809d2f94
💬 sedited commented on pull request "merkle: migrate `path` arg to reference and drop unused args":
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2589073735)
Do you think there would be value in also passing in nullptr:
```diff
diff --git a/src/test/fuzz/merkle.cpp b/src/test/fuzz/merkle.cpp
index 4bb91faf0b..5b91a97b75 100644
--- a/src/test/fuzz/merkle.cpp
+++ b/src/test/fuzz/merkle.cpp
@@ -55 +55,2 @@ FUZZ_TARGET(merkle)
- const uint256 merkle_root = ComputeMerkleRoot(tx_hashes, &mutated);
+ bool* pmutated = fuzzed_data_provider.PickValueInArray({&mutated, static_cast<bool*>(nullptr)});
+ const uint256 merkle_root = ComputeMerkleR
...
(https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2589073735)
Do you think there would be value in also passing in nullptr:
```diff
diff --git a/src/test/fuzz/merkle.cpp b/src/test/fuzz/merkle.cpp
index 4bb91faf0b..5b91a97b75 100644
--- a/src/test/fuzz/merkle.cpp
+++ b/src/test/fuzz/merkle.cpp
@@ -55 +55,2 @@ FUZZ_TARGET(merkle)
- const uint256 merkle_root = ComputeMerkleRoot(tx_hashes, &mutated);
+ bool* pmutated = fuzzed_data_provider.PickValueInArray({&mutated, static_cast<bool*>(nullptr)});
+ const uint256 merkle_root = ComputeMerkleR
...