💬 stwenhao commented on issue "signet: disk-space-DoS due to low mining difficulty":
(https://github.com/bitcoin/bitcoin/issues/33266#issuecomment-3258376429)
> and would avoid this issue without needing a (signet only) (consensus) code change.
Yes, but signet-only consensus code change can be beneficial. If your difficulty requires grinding 2^20 hashes, then I think nonce values should be masked, to make sure, that upper bits are set to zero. Then, if the difficulty will increase, the code can automatically raise that barrier, so that the nonce restrictions will be active only for difficulties requiring less than 2^32 hashes.
Because if you need to
...
(https://github.com/bitcoin/bitcoin/issues/33266#issuecomment-3258376429)
> and would avoid this issue without needing a (signet only) (consensus) code change.
Yes, but signet-only consensus code change can be beneficial. If your difficulty requires grinding 2^20 hashes, then I think nonce values should be masked, to make sure, that upper bits are set to zero. Then, if the difficulty will increase, the code can automatically raise that barrier, so that the nonce restrictions will be active only for difficulties requiring less than 2^32 hashes.
Because if you need to
...
💬 inkhaton commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3258382540)
IT doesnt have to be some imaginary illegal number. Just the binary of a jpg thats child porn. With that every person running a node in most countries can be arrested.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3258382540)
IT doesnt have to be some imaginary illegal number. Just the binary of a jpg thats child porn. With that every person running a node in most countries can be arrested.
🤔 janb84 reviewed a pull request: "ci: Checkout latest merged pulls"
(https://github.com/bitcoin/bitcoin/pull/33303#pullrequestreview-3189412062)
re ACK faeb3320952906b6147b06170059e71d7d59f4bd
Looks good !
changes sinds last ACK:
- using YAML_KEYS for templating with descriptive naming (moved from env)
(https://github.com/bitcoin/bitcoin/pull/33303#pullrequestreview-3189412062)
re ACK faeb3320952906b6147b06170059e71d7d59f4bd
Looks good !
changes sinds last ACK:
- using YAML_KEYS for templating with descriptive naming (moved from env)
💬 yuvicc commented on issue "InitError() doesn't always halt node startup when blockchain state exists":
(https://github.com/bitcoin/bitcoin/issues/33276#issuecomment-3258423195)
I dug into this issue and found that this behavior lies with the `-stopatheight` argument. This argument only stops validation of blocks until that height but doesn't stop further downloading of blocks. That might be the reason we see that it validates the remaining blocks from the index and updates the current height next time when it starts.
Running the same command `build/bin/bitcoind -datadir=demo -stopatheight=1000`, I see it has blocks till 1520 in the db.
```
2025-09-05T13:32:14Z Loadin
...
(https://github.com/bitcoin/bitcoin/issues/33276#issuecomment-3258423195)
I dug into this issue and found that this behavior lies with the `-stopatheight` argument. This argument only stops validation of blocks until that height but doesn't stop further downloading of blocks. That might be the reason we see that it validates the remaining blocks from the index and updates the current height next time when it starts.
Running the same command `build/bin/bitcoind -datadir=demo -stopatheight=1000`, I see it has blocks till 1520 in the db.
```
2025-09-05T13:32:14Z Loadin
...
💬 Crypt-iQ commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325190068)
Comment added in 7c1b388c7ab2059b562c89b1e7c7dd5b641a9cb8
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325190068)
Comment added in 7c1b388c7ab2059b562c89b1e7c7dd5b641a9cb8
💬 Crypt-iQ commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325190937)
Added this in 7c1b388c7ab2059b562c89b1e7c7dd5b641a9cb8
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325190937)
Added this in 7c1b388c7ab2059b562c89b1e7c7dd5b641a9cb8
💬 instagibbs commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325208508)
IIUC block requests are cleaned up during disconnect, so this would allow noban/manual connections to re-start trying to give us blocks vs freeze them out?
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325208508)
IIUC block requests are cleaned up during disconnect, so this would allow noban/manual connections to re-start trying to give us blocks vs freeze them out?
💬 hebasto commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3258498758)
> ... can you please point me to the discussion that you were referring to with "reintroducing Python scripts"...
The Python dependency for the `translate` build target was recently removed in https://github.com/bitcoin/bitcoin/pull/33209 by @purpleKarrot.
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3258498758)
> ... can you please point me to the discussion that you were referring to with "reintroducing Python scripts"...
The Python dependency for the `translate` build target was recently removed in https://github.com/bitcoin/bitcoin/pull/33209 by @purpleKarrot.
👍 ryanofsky approved a pull request: "Add functional test for IPC interface"
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3189572477)
Code review ACK a341e11ac92b30aac856049c696a9ac39854569d. Changes since last review: rebasing, switching to miniwallet and expanding wallet test, improving pycapnp install steps in instructions and CI.
Looking at CI changes here, it's interesting that low-level way we configure CI jobs allows dependencies to be installed that might not be used, and things to be built that are not tested. I think a more ideal config / script boundary might make the config only responsible for specifying what s
...
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3189572477)
Code review ACK a341e11ac92b30aac856049c696a9ac39854569d. Changes since last review: rebasing, switching to miniwallet and expanding wallet test, improving pycapnp install steps in instructions and CI.
Looking at CI changes here, it's interesting that low-level way we configure CI jobs allows dependencies to be installed that might not be used, and things to be built that are not tested. I think a more ideal config / script boundary might make the config only responsible for specifying what s
...
🤔 stickies-v reviewed a pull request: "wallet: reduce unconditional logging during load"
(https://github.com/bitcoin/bitcoin/pull/33299#pullrequestreview-3189575123)
Concept ACK, but I think unconditional logging in the ctor is not the right approach, suggested alternative:
<details>
<summary>git diff on 689a321976</summary>
```diff
diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
index 79e2c9688d..11d7c9029d 100644
--- a/src/wallet/init.cpp
+++ b/src/wallet/init.cpp
@@ -19,6 +19,7 @@
#include <util/moneystr.h>
#include <util/translation.h>
#include <wallet/coincontrol.h>
+#include <wallet/sqlite.h>
#include <wallet/wallet.h>
#in
...
(https://github.com/bitcoin/bitcoin/pull/33299#pullrequestreview-3189575123)
Concept ACK, but I think unconditional logging in the ctor is not the right approach, suggested alternative:
<details>
<summary>git diff on 689a321976</summary>
```diff
diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
index 79e2c9688d..11d7c9029d 100644
--- a/src/wallet/init.cpp
+++ b/src/wallet/init.cpp
@@ -19,6 +19,7 @@
#include <util/moneystr.h>
#include <util/translation.h>
#include <wallet/coincontrol.h>
+#include <wallet/sqlite.h>
#include <wallet/wallet.h>
#in
...
💬 instagibbs commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325239854)
How about something like:
// We keep the failed partialBlock to disallow processing another compact block announcement from the same
// peer for the same block. We let the full block download below continue under the same m_downloading_since timer.
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325239854)
How about something like:
// We keep the failed partialBlock to disallow processing another compact block announcement from the same
// peer for the same block. We let the full block download below continue under the same m_downloading_since timer.
💬 hebasto commented on pull request "doc: fix `LIBRARY_PATH` comment":
(https://github.com/bitcoin/bitcoin/pull/33308#discussion_r2325242873)
> I think it's clear it applies to both, given it's the same code?
Yes, that’s certainly one way to look at it. I’m willing to admit it might just be my own bias to think that an inline comment applies only to the specific line it’s attached to.
That could explain why I (mistakenly) removed the comment along with the code in https://github.com/bitcoin/bitcoin/pull/32764.
(https://github.com/bitcoin/bitcoin/pull/33308#discussion_r2325242873)
> I think it's clear it applies to both, given it's the same code?
Yes, that’s certainly one way to look at it. I’m willing to admit it might just be my own bias to think that an inline comment applies only to the specific line it’s attached to.
That could explain why I (mistakenly) removed the comment along with the code in https://github.com/bitcoin/bitcoin/pull/32764.
❤1
👍 TheCharlatan approved a pull request: "Add functional test for IPC interface"
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3189590307)
ACK a341e11ac92b30aac856049c696a9ac39854569d
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3189590307)
ACK a341e11ac92b30aac856049c696a9ac39854569d
💬 Crypt-iQ commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325267840)
Yup, like @mzumsande pointed out, this shouldn't be a change in behavior since they would previously hit the `READ_STATUS_INVALID` case.
I didn't consider noban/manual connections for stalling block download as it seems like this is already possible by triggering either of the `READ_STATUS_INVALID` cases (whether [here](https://github.com/bitcoin/bitcoin/blob/37c21ebe4078d5a7b9d8a91aca2ab8b118b9d69f/src/net_processing.cpp#L4432) or [here](https://github.com/bitcoin/bitcoin/blob/37c21ebe4078d5
...
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325267840)
Yup, like @mzumsande pointed out, this shouldn't be a change in behavior since they would previously hit the `READ_STATUS_INVALID` case.
I didn't consider noban/manual connections for stalling block download as it seems like this is already possible by triggering either of the `READ_STATUS_INVALID` cases (whether [here](https://github.com/bitcoin/bitcoin/blob/37c21ebe4078d5a7b9d8a91aca2ab8b118b9d69f/src/net_processing.cpp#L4432) or [here](https://github.com/bitcoin/bitcoin/blob/37c21ebe4078d5
...
💬 hebasto commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2325276845)
The helpful explanatory comment was removed in the recent push (faeb3320952906b6147b06170059e71d7d59f4bd).
Could it be re-added, if it’s not too much trouble?
(https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2325276845)
The helpful explanatory comment was removed in the recent push (faeb3320952906b6147b06170059e71d7d59f4bd).
Could it be re-added, if it’s not too much trouble?
💬 l0rinc commented on pull request "test/refactor: use test deque to avoid quadratic iteration":
(https://github.com/bitcoin/bitcoin/pull/33313#discussion_r2325286088)
Yeah, but two lines below we're already using +=
(https://github.com/bitcoin/bitcoin/pull/33313#discussion_r2325286088)
Yeah, but two lines below we're already using +=
💬 Crypt-iQ commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325294058)
Thanks, this is less verbose. I've given commit attribution since I copied the text verbatim.
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325294058)
Thanks, this is less verbose. I've given commit attribution since I copied the text verbatim.
💬 instagibbs commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325297443)
thanks, as a future reader I always get suspicious the longer the comment is :)
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325297443)
thanks, as a future reader I always get suspicious the longer the comment is :)
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3258611648)
I tried sending a tx with fee rate 0.1 sat/vbyte using 4327327dd07457621fe1a9c88dfbf6fccb0e8854.
I connected to 3 nodes and disconnected after receiving the PONG, and then attempted one more connection every 10 minutes.
I finally received the tx back from another peer after 13 hours and 90 connections.
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3258611648)
I tried sending a tx with fee rate 0.1 sat/vbyte using 4327327dd07457621fe1a9c88dfbf6fccb0e8854.
I connected to 3 nodes and disconnected after receiving the PONG, and then attempted one more connection every 10 minutes.
I finally received the tx back from another peer after 13 hours and 90 connections.
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325246885)
You mention it in the commit message, but I think its worth adding a detailed comment above that:
* Explains the nuance around `m_scan_key`
* Explains why we are using a `PubKeyProvider`, e.g., getting origin information without needing to write custom code
I think a comment is slightly better than a commit message as I imagine this will trip people up in the future who are looking at this code and surprised to see a private key as a member field inside a pubkey provider.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2325246885)
You mention it in the commit message, but I think its worth adding a detailed comment above that:
* Explains the nuance around `m_scan_key`
* Explains why we are using a `PubKeyProvider`, e.g., getting origin information without needing to write custom code
I think a comment is slightly better than a commit message as I imagine this will trip people up in the future who are looking at this code and surprised to see a private key as a member field inside a pubkey provider.