💬 maflcko commented on pull request "Add `SigVersion` parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1457915420)
Using `default` in a switch will disable compiler warnings and make code more brittle. See the developer notes on how to handle this.

💬 maflcko commented on pull request "refactor: Fix prevector iterator concept issues":
(https://github.com/bitcoin/bitcoin/pull/29275#issuecomment-1899114359)
Fixed libc++ CIs

💬 1440000bytes commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1899130684)
@wizkid057 I searched for the word "exploit" and found you have used it several times in your messages, Which of these opcodes is not working as implemented or [documented](https://github.com/ChrisCho-H/bitcoin/blob/0bf55345764d22c5653c0a374647aa5582a186a2/src/script/script.h)?

If you want to add another config option for certain transaction that are considered "spam" by a few users, why not open a pull request that allows users to do it without changing defaults?

I have suggested 2 approa
...

💬 Christewart commented on pull request "Add `SigVersion` parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1457935010)
I attempted to fix this in c1fe7109fc989c263c19696229c73c5982ff19e7

Is that what things should look like? From Pieter's original comment, it seemed like asserting false in the case of non tapscript sig versions are was desirable.

https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1457862849

📝 ryanofsky opened a pull request: "depends: Update libmultiprocess library to fix C++20 macos build error"
(https://github.com/bitcoin/bitcoin/pull/29276)
Fixes #29248

The std::result_of type was removed in c++20, but was being referenced in some old, unused code in the library. The issue was fixed in:

- https://github.com/chaincodelabs/libmultiprocess/pull/91

This update also includes other recent libmultiprocess changes to improve C++20 support and fix build issues:

- https://github.com/chaincodelabs/libmultiprocess/pull/89
- https://github.com/chaincodelabs/libmultiprocess/pull/90

💬 ryanofsky commented on issue "build: multiprocess compile failure on macOS":
(https://github.com/bitcoin/bitcoin/issues/29248#issuecomment-1899144292)
This should be fixed by https://github.com/chaincodelabs/libmultiprocess/pull/91 and https://github.com/bitcoin/bitcoin/pull/29276.

💬 wizkid057 commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1899169520)
> @wizkid057 I searched for the word "exploit" and found you have used it several times in your comments. Which of these [opcodes](https://en.bitcoin.it/wiki/Script#Opcodes) is not working as implemented or [documented](https://github.com/ChrisCho-H/bitcoin/blob/0bf55345764d22c5653c0a374647aa5582a186a2/src/script/script.h)?

You already know the answer to this, but are asking as if it's some kind of "gotcha" here. It is not.

Whether or not stuffing arbitrary data between valid opcodes is c
...

👍 ryanofsky approved a pull request: "Avoid returning references to mutex guarded members"
(https://github.com/bitcoin/bitcoin/pull/28774#pullrequestreview-1830531507)
Code review ACK 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b. This seems like a potentially real race condition, and the fix here is pretty simple.

I do think the PR title and description are confusing and probably causing this PR to get overlooked.

Would suggest replacing PR title and description with the title and description from the 32a9f13cb805ecf6aebb5cf4e5d92b8a47c4548b commit message which are much clearer. To make the PR comment history legible, you could include the original descript
...

📝 stickies-v opened a pull request: "RPC: access RPC arguments by name"
(https://github.com/bitcoin/bitcoin/pull/29277)
Adds string overloads for the `RPCHelpMan::Arg` and `RPCHelpMan::MaybeArg` helpers to be able to access RPC arguments by name instead of index number. Especially in RPCs with a large number of parameters, this can be quite helpful.

Example usage:
```cpp
const auto action{self.Arg<std::string>("action")};
```

Most of the LoC is adding test coverage and documentation updates. No behaviour change.

An alternative approach to #27788 with significantly less overhaul.

💬 tromp commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1899207755)
> The ability to inject arbitrary data into any system in a way that isn't explicitly documented as permitted for an expected purpose of the system is always a vulnerability and always something that should be addressed.

Bitcoin script is just a simple programming language with specific technical rules that all programs must follow. On top of that there are additional technical rules that programs must follow to fall in certain categories, like isStandard.
None of these rules can express int
...

💬 maflcko commented on pull request "rpc: Be able to access RPC parameters by name":
(https://github.com/bitcoin/bitcoin/pull/27788#issuecomment-1899208902)
Can be closed after https://github.com/bitcoin/bitcoin/pull/29277 ?

🤔 furszy reviewed a pull request: "Avoid returning references to mutex guarded members"
(https://github.com/bitcoin/bitcoin/pull/28774#pullrequestreview-1830559572)
ACK 32a9f13c

💬 furszy commented on pull request "Avoid returning references to mutex guarded members":
(https://github.com/bitcoin/bitcoin/pull/28774#discussion_r1457979323)
nit:
Could inline it
```c++
return WITH_LOCK(cs_wallet, return cb(vMasterKey));
```
Also, could pass `cb` as a const ref.

💬 stickies-v commented on pull request "refactor: remove CTxMemPool::queryHashes()":
(https://github.com/bitcoin/bitcoin/pull/29260#discussion_r1457983060)
Sorry, I misunderstood the test. Replaced now with:

```cpp
const auto new_size{WITH_LOCK(tx_pool.cs, return tx_pool.entryAll().size())};
assert(new_size < info_all.size());
```

💬 ismaelsadeeq commented on pull request "RPC: access RPC arguments by name":
(https://github.com/bitcoin/bitcoin/pull/29277#issuecomment-1899213515)
Concept ACK

💬 stickies-v commented on pull request "refactor: remove CTxMemPool::queryHashes()":
(https://github.com/bitcoin/bitcoin/pull/29260#issuecomment-1899222086)
Force pushed to re-instate the fuzz test, addressing https://github.com/bitcoin/bitcoin/pull/29260#discussion_r1457572196

💬 stickies-v commented on pull request "refactor: remove CTxMemPool::queryHashes()":
(https://github.com/bitcoin/bitcoin/pull/29260#discussion_r1457991155)
`entryAll` returning just refs has less overhead than `infoAll` so I think regardless this is a preferred approach? (even if it's not a huge difference)

💬 dergoegge commented on pull request "refactor: remove CTxMemPool::queryHashes()":
(https://github.com/bitcoin/bitcoin/pull/29260#discussion_r1457992730)
could use `infoAll` to avoid the `WITH_LOCK`?

💬 achow101 commented on issue "macOS App Notarization & Stapling":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1899228781)
Did some tests on MacOS 13.5.1. There is no difference in phone home behavior for no notarization at all, notarization without stapling, and stapled notarization. Tested with Bitcoin Core 26.0, Wireshark (notarized but not stapled), and Firefox (notarized and stapled), and did packet capture for each with Wireshark. They all resulted in communication with api.apple-cloudkit.com, which, AFAIK, is the domain for the notarization check.

To replicate this experiment, the command `spctl -a -vvv -t
...

✅ achow101 closed a pull request: "rpc: Be able to access RPC parameters by name"
(https://github.com/bitcoin/bitcoin/pull/27788)