💬 maflcko commented on pull request "test: add test for coin write failure":
(https://github.com/bitcoin/bitcoin/pull/33980#issuecomment-3595989321)
> This implementation adds comprehensive test coverage for coin database write failures in Bitcoin Core. Previously, there were **no tests** for this critical failure scenario.
This is wrong. You'll have to prove which lines/paths are uncovered and show that they are now covered.
Closing as an LLM generated "AI agent" patch. Please note that contributors are required to fully understand their authored code themselves.
Also, the explanation is obviously wrong and hallucinated.
If yo
...
(https://github.com/bitcoin/bitcoin/pull/33980#issuecomment-3595989321)
> This implementation adds comprehensive test coverage for coin database write failures in Bitcoin Core. Previously, there were **no tests** for this critical failure scenario.
This is wrong. You'll have to prove which lines/paths are uncovered and show that they are now covered.
Closing as an LLM generated "AI agent" patch. Please note that contributors are required to fully understand their authored code themselves.
Also, the explanation is obviously wrong and hallucinated.
If yo
...
💬 pinheadmz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3596044151)
Push to 5f6842030a4d82b81d640f1109c543dabd0938ef:
- More nits and suggestions from Sonarcloud
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3596044151)
Push to 5f6842030a4d82b81d640f1109c543dabd0938ef:
- More nits and suggestions from Sonarcloud
💬 hebasto commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#discussion_r2576763610)
> > -Wno-error=missing-braces
From https://github.com/bitcoin/bitcoin/actions/runs/19820405203/job/56781245720:
```
D:\a\bitcoin\bitcoin\src\test\i2p_tests.cpp(116,34): warning : suggest braces around initialization of subobject [-Wmissing-braces] [D:\a\bitcoin\bitcoin\build\src\test\test_bitcoin.vcxproj]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.26100.0\shared\ws2ipdef.h(251,33): note: expanded from macro 'IN6ADDR_LOOPBACK_INIT'
D:\a\bitcoin\bitcoin\src\test\i2p_tests.cpp(159
...
(https://github.com/bitcoin/bitcoin/pull/31507#discussion_r2576763610)
> > -Wno-error=missing-braces
From https://github.com/bitcoin/bitcoin/actions/runs/19820405203/job/56781245720:
```
D:\a\bitcoin\bitcoin\src\test\i2p_tests.cpp(116,34): warning : suggest braces around initialization of subobject [-Wmissing-braces] [D:\a\bitcoin\bitcoin\build\src\test\test_bitcoin.vcxproj]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.26100.0\shared\ws2ipdef.h(251,33): note: expanded from macro 'IN6ADDR_LOOPBACK_INIT'
D:\a\bitcoin\bitcoin\src\test\i2p_tests.cpp(159
...
⚠️ laanwj opened an issue: "build: `guix` package was removed from debian"
(https://github.com/bitcoin/bitcoin/issues/33982)
Apparently the `guix` debian package was no longer maintained, and was removed from debian recently (see https://lwn.net/Articles/1035491/). Ubuntu and other debian derivatives will likely follow.
This means we need new setup instructions for guix on debian in `contrib/guix/INSTALL.md`.
(https://github.com/bitcoin/bitcoin/issues/33982)
Apparently the `guix` debian package was no longer maintained, and was removed from debian recently (see https://lwn.net/Articles/1035491/). Ubuntu and other debian derivatives will likely follow.
This means we need new setup instructions for guix on debian in `contrib/guix/INSTALL.md`.
💬 fanquake commented on issue "build: `guix` package was removed from debian":
(https://github.com/bitcoin/bitcoin/issues/33982#issuecomment-3596149271)
Related discussion in #33574.
(https://github.com/bitcoin/bitcoin/issues/33982#issuecomment-3596149271)
Related discussion in #33574.
💬 laanwj commented on issue "build: `guix` package was removed from debian":
(https://github.com/bitcoin/bitcoin/issues/33982#issuecomment-3596166889)
Thanks! Strange that i've not noticed before. This was the first time i ran into the issue.
For most platforms we could refer to the binary installers in https://ftp.gnu.org/gnu/guix/ .
It's mainly annoying on riscv64, because GUIX doesn't ship their own binaries for that, and debian did.
(https://github.com/bitcoin/bitcoin/issues/33982#issuecomment-3596166889)
Thanks! Strange that i've not noticed before. This was the first time i ran into the issue.
For most platforms we could refer to the binary installers in https://ftp.gnu.org/gnu/guix/ .
It's mainly annoying on riscv64, because GUIX doesn't ship their own binaries for that, and debian did.
💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2576823462)
I have added a composite benchmark to measure both, added originally with resize + overwrite (as the code it measures), and in the last commit changes them to even reserve.
Remeasured it as follows:
```
for commit in 5c77b621591a800095c0afd738e4b96c47400701 8bbc90a9811ee279265ad114376eff7e852bee10; do \
git fetch origin $commit >/dev/null 2>&1 && git checkout $commit >/dev/null 2>&1 && echo "" && git log -1 --pretty='%h %s' && \
rm -rf build >/dev/null 2>&1 && cmake -B build -DBUI
...
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2576823462)
I have added a composite benchmark to measure both, added originally with resize + overwrite (as the code it measures), and in the last commit changes them to even reserve.
Remeasured it as follows:
```
for commit in 5c77b621591a800095c0afd738e4b96c47400701 8bbc90a9811ee279265ad114376eff7e852bee10; do \
git fetch origin $commit >/dev/null 2>&1 && git checkout $commit >/dev/null 2>&1 && echo "" && git log -1 --pretty='%h %s' && \
rm -rf build >/dev/null 2>&1 && cmake -B build -DBUI
...
💬 Eunovo commented on issue "BIP352 tracking issue":
(https://github.com/bitcoin/bitcoin/issues/28536#issuecomment-3596206124)
> PRs ready for review:
>
> * [Add BIP352 module (take 3) bitcoin-core/secp256k1#1698](https://github.com/bitcoin-core/secp256k1/pull/1698)
[Add BIP352 module (take 3) bitcoin-core/secp256k1#1698](https://github.com/bitcoin-core/secp256k1/pull/1698) has been replaced with [Add "silentpayments" module implementing BIP352 (take 4, limited to full-node scanning)](https://github.com/bitcoin-core/secp256k1/pull/1765)
Anyone looking to try out silentpayments on bitcoin-core can use https://github.
...
(https://github.com/bitcoin/bitcoin/issues/28536#issuecomment-3596206124)
> PRs ready for review:
>
> * [Add BIP352 module (take 3) bitcoin-core/secp256k1#1698](https://github.com/bitcoin-core/secp256k1/pull/1698)
[Add BIP352 module (take 3) bitcoin-core/secp256k1#1698](https://github.com/bitcoin-core/secp256k1/pull/1698) has been replaced with [Add "silentpayments" module implementing BIP352 (take 4, limited to full-node scanning)](https://github.com/bitcoin-core/secp256k1/pull/1765)
Anyone looking to try out silentpayments on bitcoin-core can use https://github.
...
💬 laanwj commented on issue "build: `guix` package was removed from debian":
(https://github.com/bitcoin/bitcoin/issues/33982#issuecomment-3596225176)
> A 1.5.0 release is planned for sometime in January 2026: https://codeberg.org/guix/release-planning/wiki/release-1.5.0-project/. So hopefully the situation is going to improve in future.
"Wait for 1.5.0" SGTM, too.
If it isn't re-added i think it makes sense to remove the mention of the package from the docs completely, as most people are bound to run the newest debian release. The wording of "various releases" is a bit vague, more people are likely going to run into this.
So let's keep thi
...
(https://github.com/bitcoin/bitcoin/issues/33982#issuecomment-3596225176)
> A 1.5.0 release is planned for sometime in January 2026: https://codeberg.org/guix/release-planning/wiki/release-1.5.0-project/. So hopefully the situation is going to improve in future.
"Wait for 1.5.0" SGTM, too.
If it isn't re-added i think it makes sense to remove the mention of the package from the docs completely, as most people are bound to run the newest debian release. The wording of "various releases" is a bit vague, more people are likely going to run into this.
So let's keep thi
...
🤔 rkrux reviewed a pull request: "validation: Improve warnings in case of chain corruption"
(https://github.com/bitcoin/bitcoin/pull/33553#pullrequestreview-3524622915)
Concept ACK 53ef86f0ecd4ca2b66c4e3dce591ac4718e38f0e
Adding the warning in such a case should help the user in diagnosing the issue; I have limited exposure to this area, hence refraining from giving a full a-c-k.
(https://github.com/bitcoin/bitcoin/pull/33553#pullrequestreview-3524622915)
Concept ACK 53ef86f0ecd4ca2b66c4e3dce591ac4718e38f0e
Adding the warning in such a case should help the user in diagnosing the issue; I have limited exposure to this area, hence refraining from giving a full a-c-k.
💬 rkrux commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2576795045)
Looking forward to this functional test!
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2576795045)
Looking forward to this functional test!
👍 rkrux approved a pull request: "wallet: warn against accidental unsafe older() import"
(https://github.com/bitcoin/bitcoin/pull/33135#pullrequestreview-3524889277)
lgtm ACK 189437a3dedefce3945ab2a8c05785eb283c2c6f
Shared a non-blocking nit.
(https://github.com/bitcoin/bitcoin/pull/33135#pullrequestreview-3524889277)
lgtm ACK 189437a3dedefce3945ab2a8c05785eb283c2c6f
Shared a non-blocking nit.
💬 rkrux commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2576993777)
Nit: some consistency in the warning format would be easier on the eyes.
```diff
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 8289120164..132d695a1a 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1557,11 +1557,11 @@ public:
const uint32_t raw = node.k;
const uint32_t value_part = raw & ~CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG;
if (value_part > CTxIn::SEQUENCE_LOCKTIME_MASK) {
-
...
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2576993777)
Nit: some consistency in the warning format would be easier on the eyes.
```diff
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 8289120164..132d695a1a 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1557,11 +1557,11 @@ public:
const uint32_t raw = node.k;
const uint32_t value_part = raw & ~CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG;
if (value_part > CTxIn::SEQUENCE_LOCKTIME_MASK) {
-
...
👍 l0rinc approved a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3524866538)
The latest version splits out a few trivial moves with explanations to unburden heavier commits. I particularly like how `PrivateBroadcast` methods are added commit by commit when they're actually needed.
The signal-to-noise ratio is also improved significantly now - irrelevant debug logs aren't dominating anymore.
We also finally found a way to write the priority comparator in an intuitive way that the CI gods didn't reject!
`GetHash()` calls for hash and equality have been migrated to witne
...
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3524866538)
The latest version splits out a few trivial moves with explanations to unburden heavier commits. I particularly like how `PrivateBroadcast` methods are added commit by commit when they're actually needed.
The signal-to-noise ratio is also improved significantly now - irrelevant debug logs aren't dominating anymore.
We also finally found a way to write the priority comparator in an intuitive way that the CI gods didn't reject!
`GetHash()` calls for hash and equality have been migrated to witne
...
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2577109437)
to get 100% coverage of `PrivateBroadcast`, we could add:
```C++
BOOST_CHECK(!pb.DidNodeConfirmReception(nonexistent_recipient));
```
to exercise:
https://github.com/bitcoin/bitcoin/blob/2d398050ee31db05e9222149b5e60572ac31d803/src/private_broadcast.cpp#L82
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2577109437)
to get 100% coverage of `PrivateBroadcast`, we could add:
```C++
BOOST_CHECK(!pb.DidNodeConfirmReception(nonexistent_recipient));
```
to exercise:
https://github.com/bitcoin/bitcoin/blob/2d398050ee31db05e9222149b5e60572ac31d803/src/private_broadcast.cpp#L82
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2576975959)
nit:
```suggestion
* Avoid calling this for other peers since it will degrade privacy.
```
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2576975959)
nit:
```suggestion
* Avoid calling this for other peers since it will degrade privacy.
```
💬 maflcko commented on pull request "refactor: replace manual promise with SyncWithValidationInterfaceQueue":
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2577151438)
> Nit: The `callback_set` variable seems unnecessary now.
Why would it be unnecessary now? Sure, it is harmless to call `SyncWithValidationInterfaceQueue` when the tx is in the mempool, but this will need to be mentioned
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2577151438)
> Nit: The `callback_set` variable seems unnecessary now.
Why would it be unnecessary now? Sure, it is harmless to call `SyncWithValidationInterfaceQueue` when the tx is in the mempool, but this will need to be mentioned
💬 sipa commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577234887)
doc: Add design notes for cluster mempool and explain new mempool limits
Grammar: "we can can compare" -> "we can compare"
(thanks DrahtBot LLM)
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577234887)
doc: Add design notes for cluster mempool and explain new mempool limits
Grammar: "we can can compare" -> "we can compare"
(thanks DrahtBot LLM)
💬 sipa commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577225993)
In commit "doc: Add design notes for cluster mempool and explain new mempool limits"
With these options made debug-only, maybe it's better to list the defaults?
> Transactions submitted to the mempool must not result in clusters that would
exceed the cluster limits (64 transactions and 101 kvB total per cluster).
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577225993)
In commit "doc: Add design notes for cluster mempool and explain new mempool limits"
With these options made debug-only, maybe it's better to list the defaults?
> Transactions submitted to the mempool must not result in clusters that would
exceed the cluster limits (64 transactions and 101 kvB total per cluster).
💬 sipa commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577238632)
In commit "doc: add release notes snippet for cluster mempool"
"overriden" -> "overridden"
(thanks to DrahtBot LLM)
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577238632)
In commit "doc: add release notes snippet for cluster mempool"
"overriden" -> "overridden"
(thanks to DrahtBot LLM)
💬 sipa commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577251357)
In commit "rpc: improve getmempoolcluster output"
`all_chunks.push_back(std::move(chunk))` will avoid a copy
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577251357)
In commit "rpc: improve getmempoolcluster output"
`all_chunks.push_back(std::move(chunk))` will avoid a copy