💬 jarolrod commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1287879314)
I'm catching up on the syntax for Github Actions, but wouldn't this run this CI task twice when a Pull request is opened? That seems to be the case when opening this against my fork: https://github.com/jarolrod/bitcoin/pull/2

💬 ChrisCho-H commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1670599690)
@ajtowns https://github.com/bitcoin/bitcoin/pull/28234 is for reserved opcode, but this change is for disabled opcode.
Moreover, it's not just about confusing err msg but also for the efficiency which I said as a second reason above.
I agree that we need to be conservative for consensus code, though I don't see the side effect of this change(let me know if there is!).

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1287941903)
> Building on your new approach, the non-default parameters could potentially be handled with pointers, to avoid unnecessary copy operations.

I don't think your approach works. It is not possible to "move" a const reference into a shared_ptr. It will be a copy again.

Simply using a raw pointer in your approach seems fine, though.

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1287948475)
Mind creating a pull with the outstanding feedback? :)

If not, I'll try to do it next week.

💬 MarcoFalke commented on pull request "doc: Add example of how to mix private and public keys in descriptors":
(https://github.com/bitcoin/bitcoin/pull/27414#issuecomment-1670689584)
I think this can be closed "Up for grabs"? Does someone want to pick it up?

💬 MarcoFalke commented on pull request "doc: Add example of how to mix private and public keys in descriptors":
(https://github.com/bitcoin/bitcoin/pull/27414#issuecomment-1670691549)
For reference, the docs on how to squash commits: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 ajtowns commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1670725416)
There's no difference between "reserving" an opcode and "disabling" it other than the error message that's passed through when it's used. Having different error messages for "error even if not executed" and "error only when it's executed" might make sense, but that isn't what we currently do (OP_VERIF/OP_VERNOTIF are exceptions) and isn't what this PR currently does.

Having the error codes be:

```
case SCRIPT_ERR_BAD_OPCODE:
return "Attempted to execute invalid/reserv
...

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288005314)
One is a `std::vector`, the other is `UniValue`, no?

💬 ajtowns commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1670764585)
> All of those transactions are taken from my OpenTimestamps calendars. My OTS calendars bump fees repeatedly, increasing the feerate by 1sat/vb with each replacement, starting at approximately the minimum relay fee.

Yes, that is a pretty perfect mechanism if you want to create a false positive for "full rbf" when all you're actually triggering is eviction from the mempool due to a change in minfee. For nodes that don't run "full rbf", they'll see one of the early transactions, add it to the
...

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r1288027509)
Thanks! rebased and fixed in https://github.com/bitcoin/bitcoin/pull/24539/commits/ff8b7d485e0b216a5ddf5e092ab5e2624f845fa4

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288037333)
Or rather, `std::optional<number>` for stuff that is returned by value and `String*` for stuff that is returned by reference?

💬 ChrisCho-H commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1670820372)
just out of curiosity, what's the difference between this(just removed `fExec && opcode == OP_VER` check)
```cpp
if (opcode == OP_CAT ||
opcode == OP_SUBSTR ||
opcode == OP_LEFT ||
opcode == OP_RIGHT ||
opcode == OP_INVERT ||
opcode == OP_AND ||
opcode == OP_OR ||
opcode == OP_XOR ||
opcode == OP_2MUL ||
opcode == OP_2DIV ||

...

💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1288073911)
Whoops yes sorry.

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1288117766)
Why would you open a pull request against your own fork? This is never done in this repo, so I don't see a reason why this should be optimized?

💬 ajtowns commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1670877495)
Changing nothing other than the `default` clause makes it easy to see that the only functional change is in the error code being returned. Changing the logic outside the `switch` requires more care to see that there aren't any other weird behaviours being introduced. That is, it's optimising for simplicity of review.

If you're wanting to give the same error message for `OP_VER` (which is only invalid if executed) the same as `OP_VERIF` (which is invalid even if not executed) then an even simp
...

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1288122926)
Obviously doesn't matter for this file, but if you retouch, could make this
```suggestion
# Copyright (c) 2019-present The Bitcoin Core developers
```

to avoid having to touch it ever again

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1288140474)
These are just uncaching the temporary coins (i.e. outputs of the package transactions), so they are not part of the UTXO set and definitely weren't pulled from disk.

💬 ajtowns commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#discussion_r1288145804)
I think a more precise distinction is that `OP_VER`, `OP_VERIF` and `OP_VERNOTIF` all were defined and implemented in published versions of bitcoin (like `CAT`/`SUBSTR`/etc), while `OP_RESERVED`, `OP_RESERVED1`, `OP_RESERVERD2`, `OP_INVALIDOPCODE`, `OP_PUBKEY`, `OP_PUBKEYHASH` (etc) all have had assigned names, but never had a published implementation in `EvalScript` (not to mention opcodes 0xbb through 0xf8 or so which didn't even get given names).

It doesn't seem that useful to me to have d
...

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1670943979)
I think this PR should also contain `IsInputForSharedSecretDerivation` that's currently in the send PR, as well as `SumInputPubKeys` (not sure where that's introduced). More generally: anything that's critical in deriving the correct key from a transaction, given its inputs.

💬 ChrisCho-H commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1670944317)
I think it might be better to separate `OP_VER` and its variants as functionally different(based on executed branch or not).
just throw `SCRIPT_ERR_DISABLED_OPCODE` for `OP_VERIF` and `OP_VERNOTIF`, not for `OP_VER`.
let `OP_VER` throw `SCRIPT_ERR_BAD_OPCODE` as it is.