👋 fanquake's pull request is ready for review: "[26.x] Final Changes"
(https://github.com/bitcoin/bitcoin/pull/28872)

💬 maflcko commented on pull request "test: fix `AddNode` unit test failure on OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/28891#issuecomment-1814561402)
> NetBSD and FreeBSD, `127.1` is resolved correctly

Confirmed this on FreeBSD. I don't have OpenBSD or NetBSD.

💬 maflcko commented on pull request "wallet: propagete `checkChainLimits` error message to wallet":
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1395800664)
According to iwyu, it seems better to include

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1395809829)
I don't quite follow. `hdmasterfingerprint` is not available for descriptor wallets.

📝 hebasto opened 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
...

📝 furszy opened a pull request: "wallet: batch all atomic spkms setup db writes in a single db txn "
(https://github.com/bitcoin/bitcoin/pull/28894)
Work decoupled from #28574.

Instead of performing multiple single write operations per spkm
setup call, this PR batches them all within a single atomic db txn.

Speeding up the process and preventing the wallet from entering
an inconsistent state if any of the intermediate transactions fail
(which shouldn't happen but.. if it does, it is better to not store
any spkm rather than storing them partially).

To compare the changes, added benchmark in the first commit.

💬 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_r1395831233)
Thanks for the explanations.

I've refactored out handling pushed data, as I wasn't enjoying all the nested if statements, and notice that we are currently handling "non-minimal pushes" the same as minimal pushes with non-minimally-encoded values, which is to use `OPCODE<hex>`. This seems ok, and it will not be a lossy decode, but do we want to differentiate between the two pathways to this decode format?

See first and last case in snippet to illustrate what I mean above:

```cpp
/*
*
...

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