💬 pablomartin4btc commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#issuecomment-1896372688)
Updates:
- Switch the logic strategy to the previously presented alternative due to preference of reviewers @kevkevinpal & @furszy (thanks!);
- Added test coverage

💬 totient commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1896400042)
> No matter what new restrictions are imposed, it's safe to assume that the economic reality of people wanting to inscribe data on the blockchain will persist and use whatever means is available and cheapest. There will always be means available such as spreading the data over multiple outputs or multiple txs, encoding the data as arbitrary taproot scripts, or encoding the data as fake public keys or key hashes. Some of these encodings will be detectable while others will not. Raising the cost o
...

🤔 pablomartin4btc reviewed a pull request: "rpc: Fix race in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/29262#pullrequestreview-1827979729)
ACK 5555d8db3313f893609eb0cf549bb597361d4466

✅ theuni closed a pull request: "Replace non-standard CLZ builtins with c++20's bit_width"
(https://github.com/bitcoin/bitcoin/pull/29057)

💬 theuni commented on pull request "Replace non-standard CLZ builtins with c++20's bit_width":
(https://github.com/bitcoin/bitcoin/pull/29057#issuecomment-1896546558)
@maflcko I'm thinking we should just take the c++20 code and assume it will be more performant going forward. That makes sense to me anyway.

I'm going to go ahead and close this and include it in another PR which gathers the serialization c++20 changes. We can do another round of benches there.

📝 theuni opened a pull request: "serialization: c++20 endian/byteswap/clz modernization"
(https://github.com/bitcoin/bitcoin/pull/29263)
This replaces #28674, #29036, and #29057. Now ready for testing and review.

Replaces platform-specific endian and byteswap functions. This is especially useful for kernel, as it means that our deep serialization code no longer requires bitcoin-config.h.

I apologize for the size of the last commit, but it's hard to avoid making those changes at once.

All platforms now use our internal functions rather than libc or platform-specific ones, with the exception of MSVC.

Sadly, benchmarking
...

✅ fanquake closed a pull request: "[POC] C++20 `std::endian`"
(https://github.com/bitcoin/bitcoin/pull/28674)

💬 theuni commented on pull request "Add missing byteswap functions for MSVC":
(https://github.com/bitcoin/bitcoin/pull/29036#issuecomment-1896567110)
Obsoleted by #29263.

✅ fanquake closed a pull request: "Add missing byteswap functions for MSVC"
(https://github.com/bitcoin/bitcoin/pull/29036)

💬 furszy commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#discussion_r1456421159)
I don't to see how this part of the message would be helpful to users. "file size being too small" does not tell me anything. Would be better to say that the file is corrupt and/or was crafted in an incompatible format. Also, no need to add the internal exception message to the user facing error.

📝 instagibbs opened a pull request: "Add max_tx_weight to transaction funding options"
(https://github.com/bitcoin/bitcoin/pull/29264)
This allows a transaction's weight to be bound under
a certain weight if possible an desired. This
can be beneficial for future RBF attempts, or
whenever a more restricted spend topology is
desired.

See https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418/11?u=instagibbs for more discussion.

Seeking concept ACK and maybe some help on approach for testing.

💬 pablomartin4btc commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#discussion_r1456438606)
Makes sense, thanks!

💬 brunoerg commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1896635863)
Thanks for the reviews! Since there is a consensus about being useful only for manual connections, I just changed the approach. The flags won't be applied for automatic connections, only for manual ones. I updated the title and description.

💬 brunoerg commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1456454845)
jsut changed the approach, thanks: https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1896635863

📝 Christewart opened a pull request: "Add leaf_version parameter to `IsOpSuccess()`"
(https://github.com/bitcoin/bitcoin/pull/29265)
This PR adds a `leaf_version` parameter to `IsOpSuccess()`. This is needed to make continued progress on #29171 .

### Problem
As per [BIP341](https://github.com/bitcoin/bips/blob/master/bip-0342.mediawiki#rationale)
>Given that OP_SUCCESSx even causes potentially unparseable scripts to pass, it can be used to introduce multi-byte opcodes, or even a completely new scripting language when prefixed with a specific OP_SUCCESSx opcode.

This is occurring on #29171, in [script_assets_test.json
...

💬 Christewart commented on pull request "Add leaf_version parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1456471781)
For unknown leaf_versions, do we want to return true by default?

💬 furszy commented on pull request "Avoid returning references to mutex guarded members":
(https://github.com/bitcoin/bitcoin/pull/28774#discussion_r1456489383)
These methods doesn't seems to fit inside the `WalletStorage` interface. What do you think about creating a new interface for encrypting/decrypting data, then provide it to each spkm?

💬 TheCharlatan commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#issuecomment-1896833001)
Why not change the behavior around the `CORE_CFLAGS` as well? That seems a bit confusing to me; we now have one variable that always retains `-g -O2` when overriding and another that does not.

💬 sipa commented on pull request "Add leaf_version parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#issuecomment-1896903940)
I think you'd want to pass SigVersion rather than leaf_version. The SigVersion defines what script/signature rules are active, while the leaf_version is one of several ways of encoding that information in scripts. It also answers your comment: we don't care about unknown leaf_versions because they will never occur (under an unknown leaf version, no script execution happens at all).

That said, I think a change like this belongs in the PR that implements a consensus change that needs it. It's gen
...

💬 Christewart commented on pull request "Add leaf_version parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#issuecomment-1896953936)
> That said, I think a change like this belongs in the PR that implements a consensus change that needs it. It's generally better not
to touch consensus code unless necessary.

Small quibble here, there are other consensus PRs out there that probably would need this change ( #29247 , #29050 , #28550 ).

Its unclear to me right now if I was just "lucky" with my opcode byte selection such that I ran into a test case that failed with witness redeemScript (`4edc`). Perhaps the test vectors are
...