💬 fanquake commented on issue "p2p: `seed.bitcoin.sipa.be` is down":
(https://github.com/bitcoin/bitcoin/issues/34104#issuecomment-3670729702)
cc @sipa
(https://github.com/bitcoin/bitcoin/issues/34104#issuecomment-3670729702)
cc @sipa
👋 fanquake's pull request is ready for review: "refactor: enable `readability-container-contains` clang-tidy rule"
(https://github.com/bitcoin/bitcoin/pull/34095)
(https://github.com/bitcoin/bitcoin/pull/34095)
📝 l0rinc opened a pull request: "kernel: revert accidentally removed copyright header"
(https://github.com/bitcoin/bitcoin/pull/34105)
See:
https://github.com/bitcoin/bitcoin/commit/7990463b1059ba5fc4ebe37fd1105a9e168ae20d?diff=split#diff-04e685224f1ac5bfd91d47d8d7528a2e44f94fab5535d4b6b5af79b5a13aeb93L1-L12
The author mentioned it was likely a bad rebase.
Found while reviewing https://github.com/bitcoin/bitcoin/pull/34084#issuecomment-3670396535
(https://github.com/bitcoin/bitcoin/pull/34105)
See:
https://github.com/bitcoin/bitcoin/commit/7990463b1059ba5fc4ebe37fd1105a9e168ae20d?diff=split#diff-04e685224f1ac5bfd91d47d8d7528a2e44f94fab5535d4b6b5af79b5a13aeb93L1-L12
The author mentioned it was likely a bad rebase.
Found while reviewing https://github.com/bitcoin/bitcoin/pull/34084#issuecomment-3670396535
💬 theuni commented on pull request "net: Waste less time in socket handling":
(https://github.com/bitcoin/bitcoin/pull/34025#issuecomment-3670774799)
Post-merge ACK 5f5c1ea01955d277581f6c2acbeb982949088960. Nits aside, both of these are nice cleanups.
(https://github.com/bitcoin/bitcoin/pull/34025#issuecomment-3670774799)
Post-merge ACK 5f5c1ea01955d277581f6c2acbeb982949088960. Nits aside, both of these are nice cleanups.
💬 fanquake commented on pull request "kernel: revert accidentally removed copyright header":
(https://github.com/bitcoin/bitcoin/pull/34105#issuecomment-3670801358)
ACK 85314dc0bf871226c0e43446bb79f49630d15f4a
(https://github.com/bitcoin/bitcoin/pull/34105#issuecomment-3670801358)
ACK 85314dc0bf871226c0e43446bb79f49630d15f4a
🚀 fanquake merged a pull request: "kernel: revert accidentally removed copyright header"
(https://github.com/bitcoin/bitcoin/pull/34105)
(https://github.com/bitcoin/bitcoin/pull/34105)
💬 Crypt-iQ commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2631512181)
I was assuming that directly calling `ActivateBestChain` wouldn't work in a fuzz test due to certain things not being mocked out, but I wonder if it's possible to instead call it and not have to re-implement it here?
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2631512181)
I was assuming that directly calling `ActivateBestChain` wouldn't work in a fuzz test due to certain things not being mocked out, but I wonder if it's possible to instead call it and not have to re-implement it here?
💬 Crypt-iQ commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#issuecomment-3670814276)
reACK https://github.com/bitcoin/bitcoin/commit/db2d39f642979f929261e5f1cd67f0c2f2ca045f
(https://github.com/bitcoin/bitcoin/pull/31533#issuecomment-3670814276)
reACK https://github.com/bitcoin/bitcoin/commit/db2d39f642979f929261e5f1cd67f0c2f2ca045f
💬 theuni commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2631524729)
@l0rinc As the code is written at the moment, all platforms must opt-in to vectorization as opposed to opting out. I'm not sure that's the best approach, but I figured that was a reasonable starting point.
My reasoning for that was: consider non-x86, non-arm+neon platforms. For the most part, I'm assuming they're under-powered. Enabling (for example) 4x blocks/sec for mipsel would probably cause a drastic slowdown. Obviously there are lots of other powerful platforms, but I figured those woul
...
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2631524729)
@l0rinc As the code is written at the moment, all platforms must opt-in to vectorization as opposed to opting out. I'm not sure that's the best approach, but I figured that was a reasonable starting point.
My reasoning for that was: consider non-x86, non-arm+neon platforms. For the most part, I'm assuming they're under-powered. Enabling (for example) 4x blocks/sec for mipsel would probably cause a drastic slowdown. Obviously there are lots of other powerful platforms, but I figured those woul
...
🚀 fanquake merged a pull request: "fuzz: Add fuzz target for block index tree and related validation events"
(https://github.com/bitcoin/bitcoin/pull/31533)
(https://github.com/bitcoin/bitcoin/pull/31533)
💬 sipa commented on issue "p2p: `seed.bitcoin.sipa.be` is down":
(https://github.com/bitcoin/bitcoin/issues/34104#issuecomment-3670853633)
I don't see anything wrong with it, and `check-dnsseeds.py` works for me.
(https://github.com/bitcoin/bitcoin/issues/34104#issuecomment-3670853633)
I don't see anything wrong with it, and `check-dnsseeds.py` works for me.
✅ 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
...