💬 Eunovo commented on pull request "rpc, doc: clarify watch-only wallets balances in RPCHelp":
(https://github.com/bitcoin/bitcoin/pull/32761#issuecomment-3012975690)
ACK https://github.com/bitcoin/bitcoin/pull/32761/commits/cb44680db40c85ba338f2a054511cd68ba6e89ba
(https://github.com/bitcoin/bitcoin/pull/32761#issuecomment-3012975690)
ACK https://github.com/bitcoin/bitcoin/pull/32761/commits/cb44680db40c85ba338f2a054511cd68ba6e89ba
💬 purpleKarrot commented on pull request "build: Find Boost in config mode":
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3012982665)
ACK 14653b869b91f8013656099c9eb23b3518b8e53e
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3012982665)
ACK 14653b869b91f8013656099c9eb23b3518b8e53e
💬 theStack commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3013037586)
Side-note about when to call `LimitOrphans`: IIUC, so far the discussed scenarios to exceed the global limit were focused on the obvious cases of increasing the nominator of the DoS score fraction, i.e. adding new orphans or announcements (`AddTx`/`AddAnnouncement`). What about decreasing the denominator? Disconnecting a peer decreases the global usage limit, so if remaining peers have previously exceeded their per-peer usage limit, it could lead to an exceeding of global usage limit if the disc
...
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3013037586)
Side-note about when to call `LimitOrphans`: IIUC, so far the discussed scenarios to exceed the global limit were focused on the obvious cases of increasing the nominator of the DoS score fraction, i.e. adding new orphans or announcements (`AddTx`/`AddAnnouncement`). What about decreasing the denominator? Disconnecting a peer decreases the global usage limit, so if remaining peers have previously exceeded their per-peer usage limit, it could lead to an exceeding of global usage limit if the disc
...
🤔 janb84 reviewed a pull request: "cmake: Introduce `WITH_PYTHON` build option"
(https://github.com/bitcoin/bitcoin/pull/31669#pullrequestreview-2966554130)
Concept ACK 40d935904c2b1319b863306d32fef235cd008a8e
(if we want to keep python check in CMAKE)
PR Adds build option to ignore / not fail on missing python. When python is missing or below required version the configuration will error out. This removes failing silently on python.
This PR will partial solve issue #31476 (strictly the Python issue)
---
Alternative thoughts:
Should CMAKE even test for python ? If you see CMAKE as configuration for the BUILD step and we do not use pytho
...
(https://github.com/bitcoin/bitcoin/pull/31669#pullrequestreview-2966554130)
Concept ACK 40d935904c2b1319b863306d32fef235cd008a8e
(if we want to keep python check in CMAKE)
PR Adds build option to ignore / not fail on missing python. When python is missing or below required version the configuration will error out. This removes failing silently on python.
This PR will partial solve issue #31476 (strictly the Python issue)
---
Alternative thoughts:
Should CMAKE even test for python ? If you see CMAKE as configuration for the BUILD step and we do not use pytho
...
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3013076501)
@theStack I don't think this matters really, because "number of peers with at least one orphan" is used as a logistically-easier approximation for "number of peers which participate in transaction relay" (total memory usage is allowed to go up with more peers, because it's expected that peers that do transaction relay result in increases memory usage due to that). The fact that this means that peers which don't have any announced remaining orphans are not counted is a side-effect, not the goal.
...
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3013076501)
@theStack I don't think this matters really, because "number of peers with at least one orphan" is used as a logistically-easier approximation for "number of peers which participate in transaction relay" (total memory usage is allowed to go up with more peers, because it's expected that peers that do transaction relay result in increases memory usage due to that). The fact that this means that peers which don't have any announced remaining orphans are not counted is a side-effect, not the goal.
...
💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2172062756)
@yuvicc can you un-resolve this? I'd like other reviewers to see it and yell at me if I'm wrong :)
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2172062756)
@yuvicc can you un-resolve this? I'd like other reviewers to see it and yell at me if I'm wrong :)
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3013114686)
> doing once more shouldn't introduce any noticeable overhead
Looking at `CCoinsViewCache::AccessCoin()`, it calls `FetchCoin`. This either returns a previously cached coin, or fetches it from its base (possibly on disk) and then adds it to the cache.
So the worst case overhead for a _second_ `AccessCoin()` call is having to perform another `try_emplace` on this cache.
The `CCoinsCaching` bench @l0rinc points to tests `AreInputsStandard` using a very small dummy coin cache. That's prob
...
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3013114686)
> doing once more shouldn't introduce any noticeable overhead
Looking at `CCoinsViewCache::AccessCoin()`, it calls `FetchCoin`. This either returns a previously cached coin, or fetches it from its base (possibly on disk) and then adds it to the cache.
So the worst case overhead for a _second_ `AccessCoin()` call is having to perform another `try_emplace` on this cache.
The `CCoinsCaching` bench @l0rinc points to tests `AreInputsStandard` using a very small dummy coin cache. That's prob
...
👍 instagibbs approved a pull request: "test: Fix reorg patterns in tests to use proper fork-based approach"
(https://github.com/bitcoin/bitcoin/pull/32587#pullrequestreview-2966618410)
reACK eb330b58d8d20bb4a5002906cb48bb03c5fa1a10
squashes and a couple errant comments deleted
(https://github.com/bitcoin/bitcoin/pull/32587#pullrequestreview-2966618410)
reACK eb330b58d8d20bb4a5002906cb48bb03c5fa1a10
squashes and a couple errant comments deleted
🤔 scgbckbone reviewed a pull request: "test: Add `leaf_version` parameter to `taproot_tree_helper()`"
(https://github.com/bitcoin/bitcoin/pull/29371#pullrequestreview-2966639068)
ACK
(https://github.com/bitcoin/bitcoin/pull/29371#pullrequestreview-2966639068)
ACK
💬 instagibbs commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2172077826)
For policy no way does this slowdown matter. That said, since we're already doing the computation in both policy and consensus contexts, maybe we can just reuse it? https://github.com/instagibbs/bitcoin/tree/2025-06-patho-bip54
it compiles
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2172077826)
For policy no way does this slowdown matter. That said, since we're already doing the computation in both policy and consensus contexts, maybe we can just reuse it? https://github.com/instagibbs/bitcoin/tree/2025-06-patho-bip54
it compiles
💬 fanquake commented on pull request "build: Find Boost in config mode":
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3013155618)
Guix Build:
```bash
f5e794c539bcf8260fe68be5893004f3a8f7aacbaba794704116b0fc38dd5813 guix-build-14653b869b91/output/aarch64-linux-gnu/SHA256SUMS.part
a701102c9ae49f32c780241a0dbfe16589213cbb43d7c48d8ca35de0c9032c2f guix-build-14653b869b91/output/aarch64-linux-gnu/bitcoin-14653b869b91-aarch64-linux-gnu-debug.tar.gz
e0f184484a32b5a00c8034a3cb3985e21abf6df342602e6ad4cd3063ccd12f2f guix-build-14653b869b91/output/aarch64-linux-gnu/bitcoin-14653b869b91-aarch64-linux-gnu.tar.gz
4ef01b549490fff7
...
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3013155618)
Guix Build:
```bash
f5e794c539bcf8260fe68be5893004f3a8f7aacbaba794704116b0fc38dd5813 guix-build-14653b869b91/output/aarch64-linux-gnu/SHA256SUMS.part
a701102c9ae49f32c780241a0dbfe16589213cbb43d7c48d8ca35de0c9032c2f guix-build-14653b869b91/output/aarch64-linux-gnu/bitcoin-14653b869b91-aarch64-linux-gnu-debug.tar.gz
e0f184484a32b5a00c8034a3cb3985e21abf6df342602e6ad4cd3063ccd12f2f guix-build-14653b869b91/output/aarch64-linux-gnu/bitcoin-14653b869b91-aarch64-linux-gnu.tar.gz
4ef01b549490fff7
...
💬 leopardracer commented on pull request "correct variable name in p2p_monitor.py":
(https://github.com/bitcoin/bitcoin/pull/32816#issuecomment-3013168737)
> lgtm ACK
@maflcko everything is ok?
(https://github.com/bitcoin/bitcoin/pull/32816#issuecomment-3013168737)
> lgtm ACK
@maflcko everything is ok?
🚀 fanquake merged a pull request: "build: Find Boost in config mode"
(https://github.com/bitcoin/bitcoin/pull/32667)
(https://github.com/bitcoin/bitcoin/pull/32667)
💬 hebasto commented on pull request "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)":
(https://github.com/bitcoin/bitcoin/pull/32564#issuecomment-3013197158)
Friendly ping @maflcko @theuni @darosior @fanquake who were reviewing https://github.com/bitcoin/bitcoin/pull/28657.
(https://github.com/bitcoin/bitcoin/pull/32564#issuecomment-3013197158)
Friendly ping @maflcko @theuni @darosior @fanquake who were reviewing https://github.com/bitcoin/bitcoin/pull/28657.
💬 fanquake commented on pull request "guix: warn and abort when SOURCE_DATE_EPOCH is set":
(https://github.com/bitcoin/bitcoin/pull/32678#issuecomment-3013209494)
Backported to 28.x in #32811.
(https://github.com/bitcoin/bitcoin/pull/32678#issuecomment-3013209494)
Backported to 28.x in #32811.
💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where Base64 encoding is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3013213139)
> If possible it would be good to add python tests to check the new behavior
Sure, I'm also thinking of adding a python test, but the main problem is, this issue is only reproducible when there exists `=` padding or in between, so while creating a PSBT, I need to look at how I can achieve this. Or maybe by calling RPCs with a label.
> I also agree with https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2171811426 that this change doesn't need to be limited to base64 parameters and
...
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3013213139)
> If possible it would be good to add python tests to check the new behavior
Sure, I'm also thinking of adding a python test, but the main problem is, this issue is only reproducible when there exists `=` padding or in between, so while creating a PSBT, I need to look at how I can achieve this. Or maybe by calling RPCs with a label.
> I also agree with https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2171811426 that this change doesn't need to be limited to base64 parameters and
...
💬 pinheadmz commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3013238304)
@furszy I think it makes more sense to move the validation signals to their own scheduler thread if we are really concerned scheduled events will slow down block validation. `walletpassphrase` is currently the only RPC that schedules a new event, so all this PR will add to the queue are calls to `CWallet::Lock()` after the user needs a private key (and in the GUI an entirely different timer is used for this, based in Qt!).
We can also run some benchmarks, ie unlock 1000 wallets during IBD and
...
(https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3013238304)
@furszy I think it makes more sense to move the validation signals to their own scheduler thread if we are really concerned scheduled events will slow down block validation. `walletpassphrase` is currently the only RPC that schedules a new event, so all this PR will add to the queue are calls to `CWallet::Lock()` after the user needs a private key (and in the GUI an entirely different timer is used for this, based in Qt!).
We can also run some benchmarks, ie unlock 1000 wallets during IBD and
...
💬 sipa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#issuecomment-3013244578)
The reason why `FeeFrac` supports negative fees and negative sizes is because that allows it to be used on "set differences" (*), which was at some point expected to be used inside the cluster linearization algorithm. It isn't in the current version in master, however, but it ended up being very useful in PR #32545 (see [this line](https://github.com/bitcoin/bitcoin/pull/32545/commits/68ce630abd669b86d9e2ff270aa2ba1e7332f95a#diff-1433c1fc4926a466291656ba67cf6b029523e4bd5da177ade812f25edf07343cR9
...
(https://github.com/bitcoin/bitcoin/pull/32750#issuecomment-3013244578)
The reason why `FeeFrac` supports negative fees and negative sizes is because that allows it to be used on "set differences" (*), which was at some point expected to be used inside the cluster linearization algorithm. It isn't in the current version in master, however, but it ended up being very useful in PR #32545 (see [this line](https://github.com/bitcoin/bitcoin/pull/32545/commits/68ce630abd669b86d9e2ff270aa2ba1e7332f95a#diff-1433c1fc4926a466291656ba67cf6b029523e4bd5da177ade812f25edf07343cR9
...
🤔 furszy reviewed a pull request: "rpc: use CScheduler for HTTPRPCTimer"
(https://github.com/bitcoin/bitcoin/pull/32796#pullrequestreview-2966805286)
ACK d06942c6731d5db7326bc565655b33a379a5d9b0
> I think it makes more sense to move the validation signals to their own scheduler thread if we are really concerned scheduled events will slow down block validation. walletpassphrase is currently the only RPC that schedules a new event, so all this PR will add to the queue are calls to CWallet::Lock() after the user needs a private key
Sounds fair to me.
(https://github.com/bitcoin/bitcoin/pull/32796#pullrequestreview-2966805286)
ACK d06942c6731d5db7326bc565655b33a379a5d9b0
> I think it makes more sense to move the validation signals to their own scheduler thread if we are really concerned scheduled events will slow down block validation. walletpassphrase is currently the only RPC that schedules a new event, so all this PR will add to the queue are calls to CWallet::Lock() after the user needs a private key
Sounds fair to me.
💬 Sjors commented on pull request "rpc: Handle -named argument parsing where Base64 encoding is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3013274110)
> > If possible it would be good to add python tests to check the new behavior
>
> Sure, I'm also thinking of adding a python test, but the main problem is, this issue is only reproducible when there exists `=` padding or in between, so while creating a PSBT, I need to look at how I can achieve this.
You could just manually generate a random PSBT on regtest, hardcode it into a test and call `analyzepsbt`. It doesn't matter if it's unspendable.
Or you could even just call these methods w
...
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3013274110)
> > If possible it would be good to add python tests to check the new behavior
>
> Sure, I'm also thinking of adding a python test, but the main problem is, this issue is only reproducible when there exists `=` padding or in between, so while creating a PSBT, I need to look at how I can achieve this.
You could just manually generate a random PSBT on regtest, hardcode it into a test and call `analyzepsbt`. It doesn't matter if it's unspendable.
Or you could even just call these methods w
...