💬 davidgumberg commented on pull request "build: Set AUTHOR_WARNING on warnings":
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3255946507)
I don't see how abusing the semantics of `AUTHOR_WARNING` is preferable to adding our own flag (#31665), but I prefer fixing #31476, so not a big deal.
---
I think the changes implemented by @stickies-v in https://github.com/bitcoin/bitcoin/compare/7cc9a087069bfcdb79a08ce77eb3a60adf9483af...stickies-v:2025-09/cmake-remove-configure-warnings might make sense individually, but the sum total of this approach is that it shifts the experience of building bitcoin core from `cmake -B build` doin
...
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3255946507)
I don't see how abusing the semantics of `AUTHOR_WARNING` is preferable to adding our own flag (#31665), but I prefer fixing #31476, so not a big deal.
---
I think the changes implemented by @stickies-v in https://github.com/bitcoin/bitcoin/compare/7cc9a087069bfcdb79a08ce77eb3a60adf9483af...stickies-v:2025-09/cmake-remove-configure-warnings might make sense individually, but the sum total of this approach is that it shifts the experience of building bitcoin core from `cmake -B build` doin
...
📝 hebasto opened a pull request: "clang-tidy: Disable `UndefinedBinaryOperatorResult` check in `src/ipc`"
(https://github.com/bitcoin/bitcoin/pull/33312)
The warnings are false positive and have been fixed upstream. See: https://github.com/capnproto/capnproto/pull/2334.
This PR:
1. Disables the `UndefinedBinaryOperatorResult` clang-tidy check for source files generated by the `mpgen` tool.
2. Is an alternative to the draft https://github.com/bitcoin/bitcoin/pull/33281.
3. Fixes https://github.com/bitcoin/bitcoin/issues/33256.
(https://github.com/bitcoin/bitcoin/pull/33312)
The warnings are false positive and have been fixed upstream. See: https://github.com/capnproto/capnproto/pull/2334.
This PR:
1. Disables the `UndefinedBinaryOperatorResult` clang-tidy check for source files generated by the `mpgen` tool.
2. Is an alternative to the draft https://github.com/bitcoin/bitcoin/pull/33281.
3. Fixes https://github.com/bitcoin/bitcoin/issues/33256.
💬 achow101 commented on pull request "net: Quiet down logging when router doesn't support natpmp/pcp":
(https://github.com/bitcoin/bitcoin/pull/33311#issuecomment-3255988241)
ACK 4f1a4cbccd784e25f7932f1d0293602ef7f3e814
(https://github.com/bitcoin/bitcoin/pull/33311#issuecomment-3255988241)
ACK 4f1a4cbccd784e25f7932f1d0293602ef7f3e814
✅ achow101 closed an issue: "natpmp: quiet down unconditional logging"
(https://github.com/bitcoin/bitcoin/issues/33301)
(https://github.com/bitcoin/bitcoin/issues/33301)
🚀 achow101 merged a pull request: "net: Quiet down logging when router doesn't support natpmp/pcp"
(https://github.com/bitcoin/bitcoin/pull/33311)
(https://github.com/bitcoin/bitcoin/pull/33311)
🤔 mzumsande reviewed a pull request: "net: check for empty header before calling FillBlock"
(https://github.com/bitcoin/bitcoin/pull/33296#pullrequestreview-3187440917)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33296#pullrequestreview-3187440917)
Concept ACK
💬 mzumsande commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2323675550)
> allows a peer to stall block download
I guess this would only be a serious problem in the case where all of our other peers don't support compact block (because otherwise parallel compact blocks would allow us to download the block).
In any case, this was non-obvious to me. Maybe add a comment in the spot where we fall back to `GETDATA` explaining that we don't wipe the `partialBlock` there on purpose?
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2323675550)
> allows a peer to stall block download
I guess this would only be a serious problem in the case where all of our other peers don't support compact block (because otherwise parallel compact blocks would allow us to download the block).
In any case, this was non-obvious to me. Maybe add a comment in the spot where we fall back to `GETDATA` explaining that we don't wipe the `partialBlock` there on purpose?
💬 mzumsande commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2323690854)
I think this will change behavior in non-debug builds such that repeated `blocktxn` messages will now be ignored (whereas they would result in `READ_STATUS_INVALID` / disconnection before). Shouldn't we rather keep on disconnecting the peer?
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2323690854)
I think this will change behavior in non-debug builds such that repeated `blocktxn` messages will now be ignored (whereas they would result in `READ_STATUS_INVALID` / disconnection before). Shouldn't we rather keep on disconnecting the peer?
💬 hebasto commented on issue "cmake: Errors not actually errors?":
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3256075142)
I agree that this issue needs to be fixed.
However, considering:
- the limited time left before branching off,
- the specific conditions (e.g., `-DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON`) under which the issue manifests,
- the lack of consensus on how to fix it, and
- the absence of an open PR with a suggested fix,
I propose bumping the milestone for this PR from 30.0 to 31.0.
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3256075142)
I agree that this issue needs to be fixed.
However, considering:
- the limited time left before branching off,
- the specific conditions (e.g., `-DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON`) under which the issue manifests,
- the lack of consensus on how to fix it, and
- the absence of an open PR with a suggested fix,
I propose bumping the milestone for this PR from 30.0 to 31.0.
💬 davidgumberg commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3256100512)
Conceptual question:
Why not print a warning *and* `set(ENABLE_IPC OFF)` when missing capnp? This was generally the approach take in the old autotools build system. Is it better to force the user to confront the warning and change their configuration than to just try to build something reasonable based on what dependencies the user has?
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3256100512)
Conceptual question:
Why not print a warning *and* `set(ENABLE_IPC OFF)` when missing capnp? This was generally the approach take in the old autotools build system. Is it better to force the user to confront the warning and change their configuration than to just try to build something reasonable based on what dependencies the user has?
💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323700219)
Ok, dropped this piece since this code basically never did anything at all, and adding a test that works with the wallet needs more work than I'm willing to put in right now, I'm going to leave that for all follow-up instead.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323700219)
Ok, dropped this piece since this code basically never did anything at all, and adding a test that works with the wallet needs more work than I'm willing to put in right now, I'm going to leave that for all follow-up instead.
💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323701264)
Actually, fixed.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323701264)
Actually, fixed.
💬 sipa commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3256128656)
@davidgumberg I believe that's a philosophical difference between cmakeland and autotoolsland, and I think in the case of optional build dependencies, it's better not to autodetect them, to avoid build system configuration leaking into the build (which then might not work on the system you actually want to run it on).
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3256128656)
@davidgumberg I believe that's a philosophical difference between cmakeland and autotoolsland, and I think in the case of optional build dependencies, it's better not to autodetect them, to avoid build system configuration leaking into the build (which then might not work on the system you actually want to run it on).
💬 krud491 commented on issue "natpmp: quiet down unconditional logging":
(https://github.com/bitcoin/bitcoin/issues/33301#issuecomment-3256415998)
Use markdown for format
(https://github.com/bitcoin/bitcoin/issues/33301#issuecomment-3256415998)
Use markdown for format
📝 l0rinc opened a pull request: "test/refactor: use test deque to avoid quadratic iteration"
(https://github.com/bitcoin/bitcoin/pull/33313)
Extracted from https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2323012972.
-----
In Python, [list `pop(0)` is linear](https://docs.python.org/3/tutorial/datastructures.html#using-lists-as-queues), so consuming all items in the test results in quadratic iteration.
Switching to `collections.deque` with `popleft()` expresses FIFO intent and avoids the O(n^2) path.
Behavior is unchanged - for a few hundred items the perf impact is likely negligible.
(https://github.com/bitcoin/bitcoin/pull/33313)
Extracted from https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2323012972.
-----
In Python, [list `pop(0)` is linear](https://docs.python.org/3/tutorial/datastructures.html#using-lists-as-queues), so consuming all items in the test results in quadratic iteration.
Switching to `collections.deque` with `popleft()` expresses FIFO intent and avoids the O(n^2) path.
Behavior is unchanged - for a few hundred items the perf impact is likely negligible.
💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2323760016)
Pushed to https://github.com/bitcoin/bitcoin/pull/33313, please resolve the comment
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2323760016)
Pushed to https://github.com/bitcoin/bitcoin/pull/33313, please resolve the comment
💬 fanquake commented on pull request "doc: fix `LIBRARY_PATH` comment":
(https://github.com/bitcoin/bitcoin/pull/33308#discussion_r2323762474)
I think it's clear it applies to both, given it's the same code?
(https://github.com/bitcoin/bitcoin/pull/33308#discussion_r2323762474)
I think it's clear it applies to both, given it's the same code?
💬 fanquake commented on issue "cmake: Errors not actually errors?":
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3256452263)
> the limited time left before branching off,
I don't see how that's relevant. Any fix merged into master over the next few weeks can be backported to 30.x.
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3256452263)
> the limited time left before branching off,
I don't see how that's relevant. Any fix merged into master over the next few weeks can be backported to 30.x.
⚠️ www222fff opened an issue: "Is there way to sort balance address functionality just like btcrank.site showing"
(https://github.com/bitcoin/bitcoin/issues/33314)
### Please describe the feature you'd like to see added.
Is there way to sort balance address functionality just like btcrank.site showing,
Some invest want to get the real time live balance address with sort.
### Is your feature related to a problem, if so please describe it.
_No response_
### Describe the solution you'd like
_No response_
### Describe any alternatives you've considered
_No response_
### Please leave any additional context
_No response_
(https://github.com/bitcoin/bitcoin/issues/33314)
### Please describe the feature you'd like to see added.
Is there way to sort balance address functionality just like btcrank.site showing,
Some invest want to get the real time live balance address with sort.
### Is your feature related to a problem, if so please describe it.
_No response_
### Describe the solution you'd like
_No response_
### Describe any alternatives you've considered
_No response_
### Please leave any additional context
_No response_
🤔 yuvicc reviewed a pull request: "net: Prevent node from binding to the same `CService`"
(https://github.com/bitcoin/bitcoin/pull/33231#pullrequestreview-3158265913)
ACK 1c40b32597712059d5d809925da0e9adccac0fb3
`bitcoind` stops with error message for duplicate bind options.
```
Error: Duplicate binding configuration for address 0.0.0.0:8333. Please check your -bind, -bind=...=onion and -whitebind settings.
2025-09-05T05:15:05Z torcontrol thread start
2025-09-05T05:15:05Z Loading 0 mempool transactions from file...
2025-09-05T05:15:05Z Imported mempool transactions from file: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial br
...
(https://github.com/bitcoin/bitcoin/pull/33231#pullrequestreview-3158265913)
ACK 1c40b32597712059d5d809925da0e9adccac0fb3
`bitcoind` stops with error message for duplicate bind options.
```
Error: Duplicate binding configuration for address 0.0.0.0:8333. Please check your -bind, -bind=...=onion and -whitebind settings.
2025-09-05T05:15:05Z torcontrol thread start
2025-09-05T05:15:05Z Loading 0 mempool transactions from file...
2025-09-05T05:15:05Z Imported mempool transactions from file: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial br
...