💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1395832165)
Done

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1395832532)
Done

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1395833096)
Added a test

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1395833751)
Added some comments.

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1395834444)
Done

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1395835281)
Simplified this check

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1395839164)
I think it's more futureproof to check for `ckey` since all new records involving encrypted keys have always had that suffix.

💬 sipa commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395839903)
The `<...>` and `OPCODE<...>` cases treat the pushed values as byte arrays, not as numbers, so minimal encoding or not is irrelevant.

💬 willcl-ark commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395846308)
Sure, but what I mean is that in my current impl there are _two_ ways we can end up with `OPCODE<...>`:

1) Wasn't a minimal push
2) Was a minimal push <5 bytes, but wasn't a minimally-encoded decimal value

📝 hebasto converted_to_draft a pull request: "Fix SSE4.1-related issues"
(https://github.com/bitcoin/bitcoin/pull/28893)
1. Fix the test for SSE4.1 intrinsics during build system configuration, which currently can be false positive, for example, when `CXXFLAGS="-mno-sse4.1"` provided.

This PR fixes the test by adding the `_mm_blend_epi16` SSE4.1 function used in our codebase.

2. Guard `sha_x86_shani.cpp` code with `ENABLE_SSE41` macro as it uses the `_mm_blend_epi16` function from
the SSE4.1 instruction set.

It is possible that SHA-NI is enabled even when SSE4.1 is disabled, which causes compile errors
...

💬 sipa commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395859701)
Hmm, right. I think that's redundant.

I'd go for something like:
* If minimal push, not over 5 bytes, and minimally encoded, use decimal.
* Otherwise, if minimal push, or direct push, use `<...>`.
* Otherwise, use `OPCODE<...>`.

💬 theStack commented on pull request "test: fix `AddNode` unit test failure on OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/28891#discussion_r1395859889)
Thanks, done. At the time of writing this comment I was sure that this also affects the other BSDs, but apparently it doesn't. Also fixed the commit message accordingly.

💬 dergoegge commented on pull request "fuzz: Delete wallet_notifications":
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1814640245)
> Maybe https://github.com/bitcoin/bitcoin/pull/28578 (fuzz: add target for DescriptorScriptPubKeyMan by brunoerg) could be reviewed, and confirmed that it can also catch the regression, or be adjusted to catch the regression?

What was the regression? The initial PR https://github.com/bitcoin/bitcoin/pull/23334 does not mention it.

👍 maflcko approved a pull request: "Remove version field from GetSerializeSize"
(https://github.com/bitcoin/bitcoin/pull/28878#pullrequestreview-1734526484)
Some nits in the first commit. Otherwise:

ACK 83986f464c59a6517f790a960a72574e167f3f72 📒

<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=
tr
...

💬 maflcko commented on pull request "Remove version field from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1395782019)
nit in efa9eb6d7c8012fe4ed85699d81c8fe5dd18da1e: `inline` is implied by `template` and can be removed.

💬 maflcko commented on pull request "Remove version field from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1395784487)
Why remove the `explicit`? Shouldn't matter in this context, but when in doubt, I'd prefer to keep it, unless there is a reason to remove it?

💬 maflcko commented on pull request "Remove version field from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1395789229)
Same (`inline`)

💬 maflcko commented on pull request "Remove version field from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1395786545)
unrelated nit in the first commit: Can remove the `()`, which achieve nothing in this context.

💬 maflcko commented on pull request "fuzz: Delete wallet_notifications":
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1814649851)
Ah, looks like it was never merged https://github.com/bitcoin/bitcoin/pull/23444

💬 laanwj commented on pull request "Fix SSE4.1-related issues":
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-1814651122)
Concept ACK