💬 sipa commented on pull request "Add `SigVersion` parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1457862849)
I would suggest a switch case, over all possible SigVersions. The non-taproot ones can just contain an assert false, as `IsOpSuccess` shouldn't be called for anything non-taproot.

Alternatively, there could also be separate `IsOpSuccessTapscript` and `IsOpSuccessFooscript` functions, which get called in the respective branches.

💬 Christewart commented on pull request "Add `SigVersion` parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1457864400)
Ok, both of these make sense to me. I'll do the switch implementation for now.

💬 Christewart commented on pull request "Add `SigVersion` parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#discussion_r1457881873)
This triggers the linter

>A new circular dependency in the form of "script/interpreter -> script/script -> script/interpreter" appears to have been introduced.

Is there an easy way to get around this? The include is for `SigVersion`

🤔 murchandamus reviewed a pull request: "Add max_tx_weight to transaction funding options"
(https://github.com/bitcoin/bitcoin/pull/29264#pullrequestreview-1830384740)
Concept ACK, got some suggestions how you can give Knapsack a bit of a fighting chance, although we should get rid of Knapsack instead of improving it.

💬 murchandamus commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457887218)
I would suggest that you pass the `max_weight` as an additional parameter to `ApproximateBestSubset(…)` and keep track of the current weight (e.g. `nTotalWeight`) in parallel to `nTotal` and only designate a new `nBest` and `vfBest` if it’s not just better but also a permitted weight:

```diff
- if (nTotal < nBest)
+ if (nTotal < nBest && nTotalWeight <= max_weight)
```

for extra credit you could also deselect not just
...

💬 murchandamus commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457876258)
Does 0 even ever make sense?

💬 murchandamus commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457871015)
In line 331:

```diff
- } else if (!lowest_larger || group.GetSelectionAmount() < lowest_larger->GetSelectionAmount()) {
+ } else if (group.m_weight <= max_weight && (!lowest_larger || group.GetSelectionAmount() < lowest_larger->GetSelectionAmount())) {
```

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