💬 maflcko commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453627918)
need to remove include as well, no?

💬 TheCharlatan commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453636097)
Yes.

💬 Sjors commented on pull request "build: depends move macOS C(XX) FLAGS out of C & CXX":
(https://github.com/bitcoin/bitcoin/pull/29233#issuecomment-1894038057)
I was able to make and run a depends build on Intel macOS 14.2.1 for cbc9bf11fe84deb96daf9b97a8e7499979360db2.

I'm also baking a Guix build.

💬 maflcko commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453639815)
Seems better to leave as-is, or remove the comment, if it is no longer accurate?

💬 maflcko commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#issuecomment-1894042338)
lgtm ACK 2cb38d93a8fea16bf84b072a90ac023ef345134d 🍂

<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: lgtm ACK 2cb38d93a8fea16b
...

💬 TheCharlatan commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453643762)
Will leave as is then. Seems likely that something will end up here.

👍 fanquake approved a pull request: "refactor: C++20: Use std::rotl"
(https://github.com/bitcoin/bitcoin/pull/29085#pullrequestreview-1824029448)
ACK 60446285436da62adef1c0a9b11c3336d82b4d89

Had a look at the difference in master vs this PR (rebased) on aarch64. Seems like for the most part, this gives the same output +- a handful of instructions. (Identical is identical minus difference in addresses / argument ordering etc).

| Function | GCC 13.2 | Clang 17.0.6 |
| --- | --- | --- |
| MurmurHash3 | Identical | PR gives 5 less instructions: https://www.diffchecker.com/Jmlpl4rG/ |
| ChaCha20Aligned::Keystream | PR gives 1 less in
...

💬 sipa commented on pull request "refactor: C++20: Use std::rotl":
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1894084086)
@fanquake Which side of the diff is which version?

💬 fanquake commented on pull request "refactor: C++20: Use std::rotl":
(https://github.com/bitcoin/bitcoin/pull/29085#issuecomment-1894088819)
> Which side of the diff is which version?

Left is master @ f1fcc9638cde7664b9642018fe6872148bbb0172 , right was the PR based on f1fcc9638cde7664b9642018fe6872148bbb0172.

💬 murchandamus commented on pull request "test: Add algo assert to bnb_search_test":
(https://github.com/bitcoin/bitcoin/pull/29206#issuecomment-1894112305)
In the test case the `9` is pre-selected and therefore has to be used. This is why BnB returns 9+1 instead of 10.

💬 TheCharlatan commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453696973)
clang-tidy doesn't like it being left there. Its suggestion of defaulting it in the header seems counter to what we want to achieve here.
```
/home/drgrid/bitcoin/src/./kernel/context.h:22:5: error: class 'Context' can be made trivially destructible by defaulting the destructor on its first declaration [performance-trivially-destructible,-warnings-as-errors]
~Context();
^
= default
kernel/context.cpp:23:10: note: destructor definition is here
Context::~Context() = d
...

🤔 murchandamus reviewed a pull request: "test: Add algo assert to bnb_search_test"
(https://github.com/bitcoin/bitcoin/pull/29206#pullrequestreview-1824147084)
Concept NACK

Tests should ensure that the expected outcome is achieved. The expected outcome here is that the coin selection process returns the input set that has the best waste score.

I don’t think it’s useful to require that the best input set was produced by BnB. While in our current implementation the best input set is guaranteed to be found by BnB, it is possible that future coin selection improvements would enable another algorithm to produce the same result. As such, requiring tha
...

💬 theuni commented on issue "ubsan: misaligned-pointer-use in crc32c/src/crc32c_arm64.cc":
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1894141296)
FWIW @hebasto's solution looks correct (and necessary) to me.

An alternative would be to use c++20's `std::assume_aligned` if we could guarantee the input's alignment, but looking at the usage that's definitely not the case.

💬 jamesob commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1894142760)
Approach ACK; seems like a reasonable feature reasonably implemented. Will do a more formal review soon.

💬 ns-xvrn commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1894154090)
I strongly think that datacarriersize should be updated what it truly supposed to do in the first place(not what it's documentation was changed to after ord spam started). This is like saying that every option that exists now literally doesn't apply to future additions in the functionality.
And people who say it's a cat and mouse game, isn't every attack mitigation in network security like that?
May be you want to admit that there isn't a good solution you've found but even then what makes yo
...

💬 andrewtoth commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1894164176)
ACK 79e10351b1ce1f8db98ece67aacc24f323008b37

💬 fanquake commented on issue "ubsan: misaligned-pointer-use in crc32c/src/crc32c_arm64.cc":
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1894168783)
@hebasto Can you update https://github.com/bitcoin-core/crc32c-subtree/pull/6 so that the commit includes a description of the problem, an explanation of the fix, and any further information. We should probably add a comment inline explaining why a memcpy is being added to that code.

💬 theuni commented on issue "ubsan: misaligned-pointer-use in crc32c/src/crc32c_arm64.cc":
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1894185292)
It'd be worth taking upstream as well.

💬 fanquake commented on issue "ubsan: misaligned-pointer-use in crc32c/src/crc32c_arm64.cc":
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1894191530)
> It'd be worth taking upstream as well.

We could open a PR, but I'm not sure if we'll get any traction there. The last change upstream https://github.com/google/crc32c was ~ 3 years ago, and the readme recommends the use of https://github.com/google/highwayhash, which is actively maintained.

💬 achow101 commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1894209088)
ACK df30247705940c50c5eaafd74e2abbeb8b0cec07