💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2629157441)
Sorry, I meant just so you get an error if you're using a compiler ignores the `__attribute__` entirely but still passes the `#if` in _base.cpp.

💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2629159079)
Yeah, I was thinking about whether the code should just do the non-vectorized stuff to get past the overflow then immediately go back to vectorizing, rather than waiting for the next call. I think what you've got makes sense.

💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#issuecomment-3667913915)
> So.. I'd _really_ prefer to keep things as simple here as possible. I know the recursive template functions aren't exactly pretty, but I don't think they're _THAT_ bad? Plus, with c++26, it all goes away in favor of `template for` :)

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 lo
...

💬 ajtowns commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2629220635)
Writing this as:

```c++
template<size_t... ITER> using iseq = std::index_sequence<ITER>;
static constexpr ISEQ = std::make_index_sequence<I>();

template<size_t... ITER>
ALWAYS_INLINE void arr_set_vec256(iseq<ITER>, std::array<vec256, I>& arr, const vec256& vec)
{
((std::get<ITER>(arr) = vec), ...);
}

...
arr_set_vec256(ISEQ, arr0, num256);
```

doesn't seem too bad, and avoids lambdas? Possibly still a bit clumsy if you can't capt
...

🤔 ajtowns reviewed a pull request: "policy: Remove individual transaction <minrelay restriction"
(https://github.com/bitcoin/bitcoin/pull/33892#pullrequestreview-3590460575)
Subject is a bit confusing; this leaves the checks for ***individual transactions*** considered on their own, but removes the check for transactions ***within a package***, where the transaction's feerate is below minfee, even if its got a CPFP relationship that bumps the fee. We still check the overall package fee, and we require the package to be a child plus direct parents via `IsChildWithParents`, so shouldn't end up with us adding garbage to the mempool that's immediately discarded, and the
...

💬 ajtowns commented on pull request "policy: Remove individual transaction <minrelay restriction":
(https://github.com/bitcoin/bitcoin/pull/33892#discussion_r2629291543)
I think package relay is still restricted to 1P1C, so this only means that a single child can CPFP a below-minfee parent even if the child is non-TRUC. Might be good to be clearer about that.

I think the submitpackage RPC is slightly more general, in that it will accept n+1 transactions made up of a single child and n parents, and will now support parents below minfee. However, these will not relay if there is more than one parent that is below minfee. This might be something of a foot-gun.

💬 ajtowns commented on pull request "log: Use `__func__` for -logsourcelocations":
(https://github.com/bitcoin/bitcoin/pull/34088#issuecomment-3668232092)
ACK facd3d56ccbe2414a5f2b75be7132cd8b904f1e9 -- those long signatures are terrible

⚠️ davidgumberg opened an issue: "IBD stalls if addnode peer is in IBD"
(https://github.com/bitcoin/bitcoin/issues/34096)
Starting with two fresh nodes A and B. If you start node A and before it completes IBD start a second node B with `-addnode=$ADDRESS_OF_NODE_A`, node B will stall and not proceed in IBD.

```bash
mkdir -p /tmp/nodea /tmp/nodeb

cat > /tmp/nodea/bitcoin.conf << EOF
prune=1000
bind=127.0.0.1:8333
EOF

cat > /tmp/nodeb/bitcoin.conf << EOF
prune=1000
rpcport=18331
bind=127.0.0.1:18335
addnode=127.0.0.1:8333
EOF
```

and then start both nodes however you prefer, e.g.

```
bitcoind -datadir=/tmp/nodea
...

⚠️ Svki77777 opened an issue: "good"
(https://github.com/bitcoin/bitcoin/issues/34097)
gg

💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2629585169)
Correct we don't run into exceptions here but its better to keep for defensive reason. see https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508041569

💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2629656192)
I think the previous approach seems like a better match and consistent with the rest of the codebase.

💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2629659019)
friendly ping! @sedited.

💬 maflcko commented on pull request "Add initial vectorized chacha20 implementation for 2-3x speedup":
(https://github.com/bitcoin/bitcoin/pull/34083#discussion_r2629716761)
Actually, I ran the CI config locally, but it didn't catch this, as the platform opts out. The CI doesn't run the ` sed -i "s/# define CHACHA20_VEC_DISABLE_STATES_2/\/\/# define CHACHA20_VEC_DISABLE_STATES_2/" src/crypto/chacha20_vec_base.cpp && \` portion, so it would not have caught this, even if the task was run.

💬 maflcko commented on pull request "wallet: Allow read-only database access for info and dump commands":
(https://github.com/bitcoin/bitcoin/pull/32685#issuecomment-3668646866)
merging is not done in this repo to solve conflicts. Please use a rebase, as explained in the (now deleted) drahtbot comment:

> 🐙 This pull request conflicts with the target branch and [needs rebase](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes).

💬 maflcko commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3668673323)
Only change is replacing the sed commands with a large patch.

review ACK 56750c4f87d089c6a3f093eb2bf2edd07170d4a8 🌄

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1
...

💬 maflcko commented on pull request "qa: Account for unset errno in ConnectionResetError":
(https://github.com/bitcoin/bitcoin/pull/33875#issuecomment-3668757736)
review ACK 76e0e6087d0310ec31f43d751de60adf0c0a2faa 🌬

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 76e0e6087d03
...

💬 maflcko commented on pull request "qa: Account for unset errno in ConnectionResetError":
(https://github.com/bitcoin/bitcoin/pull/33875#issuecomment-3668758592)
tested via:

```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 6cf47eba73..a43436ef9c 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -263,6 +263,7 @@ std::string RequestMethodString(HTTPRequest::RequestMethod m)
/** HTTP request callback */
static void http_request_cb(struct evhttp_request* req, void* arg)
{
+ assert(false);
evhttp_connection* conn{evhttp_request_get_connection(req)};
// Track active requests
{

💬 maflcko commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2629994409)
> The change seems unsafe because none of the other functions currently calling `value()` are switched to use `operator*` instead, so they will now throw instead of assert if the value is not set. This would make it dangerous to swap the `util::Expected` class with `std::expected` in the future.

I understand that switching `util::Expected::operator*` to `std::expected::operator*` is unsafe, but this is documented in https://en.cppreference.com/w/cpp/utility/expected/operator%252A.html:

> I
...

💬 stringintech commented on pull request "p2p: Allow block downloads from peers without snapshot block after assumeutxo validation":
(https://github.com/bitcoin/bitcoin/pull/33604#issuecomment-3669209803)
9f946a7 to 6b92a69 - rebased, resolved conflict with #30214, and addressed https://github.com/bitcoin/bitcoin/pull/33604#discussion_r2628258598. Also PR and first commit descriptions are updated.

💬 Chand-ra commented on pull request "refactor: enable `readability-container-contains` clang-tidy rule":
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3669342761)
I performed a quick `grep '\.count()'` and it looks like there are quite a few instances that seem eligible for this change but aren't included here, is there some reason for doing so? For example, I found [1](https://github.com/bitcoin/bitcoin/blob/ab513103df8d1cadd2cf23397a39c652f3a2a550/src/net.cpp#L2017), [2](https://github.com/bitcoin/bitcoin/blob/ab513103df8d1cadd2cf23397a39c652f3a2a550/src/net.cpp#L2018), [3](https://github.com/bitcoin/bitcoin/blob/ab513103df8d1cadd2cf23397a39c652f3a2a550
...

💬 janb84 commented on pull request "refactor: enable `readability-container-contains` clang-tidy rule":
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3669409766)
> I performed a quick `grep '\.count()'` and it looks like there are quite a few instances that seem eligible for this change but aren't included here, is there some reason for doing so? For example, I found [1](https://github.com/bitcoin/bitcoin/blob/ab513103df8d1cadd2cf23397a39c652f3a2a550/src/net.cpp#L2017), [2](https://github.com/bitcoin/bitcoin/blob/ab513103df8d1cadd2cf23397a39c652f3a2a550/src/net.cpp#L2018), [3](https://github.com/bitcoin/bitcoin/blob/ab513103df8d1cadd2cf23397a39c652f3a2a5
...