✅ fanquake closed an issue: "p2p: `seed.bitcoin.sipa.be` is down"
(https://github.com/bitcoin/bitcoin/issues/34104)
(https://github.com/bitcoin/bitcoin/issues/34104)
💬 fanquake commented on issue "p2p: `seed.bitcoin.sipa.be` is down":
(https://github.com/bitcoin/bitcoin/issues/34104#issuecomment-3670863688)
Could be a local issue.
(https://github.com/bitcoin/bitcoin/issues/34104#issuecomment-3670863688)
Could be a local issue.
💬 joshdoman commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#discussion_r2631558465)
Hmm, I'm open to removing the word "efficient." I included it because `PrecomputedTransactionData` is strictly unnecessary for non-taproot input verification, but it does make verification faster over multiple inputs.
That may be self-explanatory. If so, we can definitely change the description to `Create precomputed transaction data for script verification.`
(https://github.com/bitcoin/bitcoin/pull/33891#discussion_r2631558465)
Hmm, I'm open to removing the word "efficient." I included it because `PrecomputedTransactionData` is strictly unnecessary for non-taproot input verification, but it does make verification faster over multiple inputs.
That may be self-explanatory. If so, we can definitely change the description to `Create precomputed transaction data for script verification.`
👍 ismaelsadeeq approved a pull request: "http: replace WorkQueue and single threads handling for ThreadPool"
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3593444540)
Code review ACK 2de0ce5cd85e1b99e318883964df318ffb615fe4 👾
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3593444540)
Code review ACK 2de0ce5cd85e1b99e318883964df318ffb615fe4 👾
💬 ismaelsadeeq commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2631432438)
In "util: introduce general purpose thread pool”
258518d880e81938fcd9672b37bc640e8a06c930
This comment is incorrect because this check alone is not sufficient to guarantee that `Stop()` is never called from a worker thread.
Consider the following scenario: `Stop()` is called both from a worker thread and from a non-worker thread sequentially. The call from outside the pool may execute first (unless the caller explicitly waits on the future from the thread pool `Stop()`). The non thread pool
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2631432438)
In "util: introduce general purpose thread pool”
258518d880e81938fcd9672b37bc640e8a06c930
This comment is incorrect because this check alone is not sufficient to guarantee that `Stop()` is never called from a worker thread.
Consider the following scenario: `Stop()` is called both from a worker thread and from a non-worker thread sequentially. The call from outside the pool may execute first (unless the caller explicitly waits on the future from the thread pool `Stop()`). The non thread pool
...
💬 maflcko commented on pull request "netif: fix compilation warning in QueryDefaultGatewayImpl()":
(https://github.com/bitcoin/bitcoin/pull/34093#issuecomment-3670881044)
> I am not sure why there is no "comparison of integers of different signs" on Linux:
>
> ```c++
> #define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
> (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
> (nlh)->nlmsg_len <= (len))
> ```
>
> We pass `int64_t` for `len`. The first comparison checks it against `(int)sizeof(...`, but the 3rd comparison checks it against `nlmsg_len` which is `__u32`, so no matter what t
...
(https://github.com/bitcoin/bitcoin/pull/34093#issuecomment-3670881044)
> I am not sure why there is no "comparison of integers of different signs" on Linux:
>
> ```c++
> #define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
> (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
> (nlh)->nlmsg_len <= (len))
> ```
>
> We pass `int64_t` for `len`. The first comparison checks it against `(int)sizeof(...`, but the 3rd comparison checks it against `nlmsg_len` which is `__u32`, so no matter what t
...
💬 Bicaru20 commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#issuecomment-3670886818)
ACK https://github.com/bitcoin/bitcoin/commit/1841bf9cb67bca99273b3efa38633113422f5882
(https://github.com/bitcoin/bitcoin/pull/34039#issuecomment-3670886818)
ACK https://github.com/bitcoin/bitcoin/commit/1841bf9cb67bca99273b3efa38633113422f5882
💬 hodlinator commented on pull request "miniscript refactor: Remove unique_ptr-indirection":
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3670894292)
Rebased to solve trivial conflict with #33192.
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3670894292)
Rebased to solve trivial conflict with #33192.
💬 billymcbip commented on pull request "test: Improve code coverage for pubkey checks":
(https://github.com/bitcoin/bitcoin/pull/34099#issuecomment-3670937312)
@rkrux perhaps my force push on the other PR broke the code coverage job.
By the way, I've also verified that the branches are called by adding `assert(false)` to those branches. `script_tests` then failed as expected.
(https://github.com/bitcoin/bitcoin/pull/34099#issuecomment-3670937312)
@rkrux perhaps my force push on the other PR broke the code coverage job.
By the way, I've also verified that the branches are called by adding `assert(false)` to those branches. `script_tests` then failed as expected.
💬 theuni commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#issuecomment-3670950989)
> Could we get some comments in the code as to compiler versions (and architecture) that failed to be efficient enough with lambdas, to hopefully avoid future people modernizing the code without checking that these problems have gone away? Presumably will be a long time before we can modernize to `template for` (which doesn't even seem to be on [cppref's compiler support page](https://en.cppreference.com/w/cpp/compiler_support.html) yet?)...
>
> (Only thing that I do think is "that bad" about
...
(https://github.com/bitcoin/bitcoin/pull/34083#issuecomment-3670950989)
> Could we get some comments in the code as to compiler versions (and architecture) that failed to be efficient enough with lambdas, to hopefully avoid future people modernizing the code without checking that these problems have gone away? Presumably will be a long time before we can modernize to `template for` (which doesn't even seem to be on [cppref's compiler support page](https://en.cppreference.com/w/cpp/compiler_support.html) yet?)...
>
> (Only thing that I do think is "that bad" about
...
🚀 fanquake merged a pull request: "[30.x] Finalise v30.1"
(https://github.com/bitcoin/bitcoin/pull/34092)
(https://github.com/bitcoin/bitcoin/pull/34092)
💬 anuragchvn-blip commented on pull request "doc: Use multipath descriptors in descriptors.md and linked test":
(https://github.com/bitcoin/bitcoin/pull/34100#issuecomment-3670966160)
@rkrux Thank you for the review and testing!
I've pushed a fix addressing both issues:
1. **Test failure**: Added `"range": 1000` parameter to `importdescriptors` to enable change address generation. The multipath descriptor now properly expands to both receive and change paths.
2. **Documentation clarity**: Changed `(0)` and `(1)` to `(/0)` and `(/1)` notation as suggested - much clearer in context.
The test should now pass. Could you re-test when you have a chance? Thanks again fo
...
(https://github.com/bitcoin/bitcoin/pull/34100#issuecomment-3670966160)
@rkrux Thank you for the review and testing!
I've pushed a fix addressing both issues:
1. **Test failure**: Added `"range": 1000` parameter to `importdescriptors` to enable change address generation. The multipath descriptor now properly expands to both receive and change paths.
2. **Documentation clarity**: Changed `(0)` and `(1)` to `(/0)` and `(/1)` notation as suggested - much clearer in context.
The test should now pass. Could you re-test when you have a chance? Thanks again fo
...
💬 ismaelsadeeq commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#issuecomment-3670973117)
The rebase is trivial it seems https://github.com/bitcoin/bitcoin/compare/master...ismaelsadeeq:bitcoin:12-2025-threadpool
(https://github.com/bitcoin/bitcoin/pull/33689#issuecomment-3670973117)
The rebase is trivial it seems https://github.com/bitcoin/bitcoin/compare/master...ismaelsadeeq:bitcoin:12-2025-threadpool
💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2631659231)
I agree that it would be nice, but it's not so simple in my opinion:
`ConnectTip()` being protected or not seems to be the least of problems:
1. It would need to become `virtual`, because it is called internally by `ABCStep`. `ConnectTip()` is a member of `Chainstate`, not `ChainstateManager`.
2. even if `ConnectTip` was virtual, it won't help with the current way to cast to a derived type (`auto& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);`) - which seems to be
...
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2631659231)
I agree that it would be nice, but it's not so simple in my opinion:
`ConnectTip()` being protected or not seems to be the least of problems:
1. It would need to become `virtual`, because it is called internally by `ABCStep`. `ConnectTip()` is a member of `Chainstate`, not `ChainstateManager`.
2. even if `ConnectTip` was virtual, it won't help with the current way to cast to a derived type (`auto& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);`) - which seems to be
...
🤔 marcofleon reviewed a pull request: "refactor: enable `readability-container-contains` clang-tidy rule"
(https://github.com/bitcoin/bitcoin/pull/34095#pullrequestreview-3593752904)
code review ACK a8a0a0ff77309a95645f6de00595c1dc39cd6eff
Tested with boost 1.74, good to go.
(https://github.com/bitcoin/bitcoin/pull/34095#pullrequestreview-3593752904)
code review ACK a8a0a0ff77309a95645f6de00595c1dc39cd6eff
Tested with boost 1.74, good to go.
💬 maflcko commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2631690440)
No, I think you are right here. There probably isn't a way (even when using UB) to mock this without touching the validation module.
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2631690440)
No, I think you are right here. There probably isn't a way (even when using UB) to mock this without touching the validation module.
👍 pablomartin4btc approved a pull request: "refactor: enable `readability-container-contains` clang-tidy rule"
(https://github.com/bitcoin/bitcoin/pull/34095#pullrequestreview-3593810848)
cr-ACK a8a0a0ff77309a95645f6de00595c1dc39cd6eff
Verified the revert of `boost::multi_index::contains` instances (2).
(https://github.com/bitcoin/bitcoin/pull/34095#pullrequestreview-3593810848)
cr-ACK a8a0a0ff77309a95645f6de00595c1dc39cd6eff
Verified the revert of `boost::multi_index::contains` instances (2).
💬 l0rinc commented on pull request "miniscript refactor: Remove unique_ptr-indirection":
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3671068636)
Rebased locally, soft reset against re-rebased PR HEAD is empty.
ACK 7909f9f7d6d780f0cf2a1e94267b71157122b0f5
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3671068636)
Rebased locally, soft reset against re-rebased PR HEAD is empty.
ACK 7909f9f7d6d780f0cf2a1e94267b71157122b0f5
💬 fjahr commented on pull request "Prevent file descriptor exhaustion from too many RPC calls":
(https://github.com/bitcoin/bitcoin/pull/27731#issuecomment-3671141568)
Closing since it becomes increasingly unlikely that we will see a 2.2 release or that we might want to upgrade to it even when it comes. Additional context: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2025-12-18#1180379;
(https://github.com/bitcoin/bitcoin/pull/27731#issuecomment-3671141568)
Closing since it becomes increasingly unlikely that we will see a 2.2 release or that we might want to upgrade to it even when it comes. Additional context: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2025-12-18#1180379;
✅ fjahr closed a pull request: "Prevent file descriptor exhaustion from too many RPC calls"
(https://github.com/bitcoin/bitcoin/pull/27731)
(https://github.com/bitcoin/bitcoin/pull/27731)
🤔 janb84 reviewed a pull request: "refactor: enable `readability-container-contains` clang-tidy rule"
(https://github.com/bitcoin/bitcoin/pull/34095#pullrequestreview-3593897430)
cr ACK a8a0a0ff77309a95645f6de00595c1dc39cd6eff
Also confirmed that clang-tidy (still) works
(https://github.com/bitcoin/bitcoin/pull/34095#pullrequestreview-3593897430)
cr ACK a8a0a0ff77309a95645f6de00595c1dc39cd6eff
Also confirmed that clang-tidy (still) works