💬 ajtowns commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1670499129)
> I have not lost interest I simply do not have the technical expertise to maintain this

Closing and marking as up for grabs.

✅ ajtowns closed a pull request: "Remove -mempoolfullrbf option"
(https://github.com/bitcoin/bitcoin/pull/26525)

💬 ajtowns commented on pull request "net_processing: re-allow fetching of genesis block via `getblockfrompeer`":
(https://github.com/bitcoin/bitcoin/pull/28205#issuecomment-1670573469)
Concept NACK, I think -- why use the network to retrieve something that's already hardcoded? Easy to special case this in `getblock` I think:

```c++
static CBlock GetBlockChecked(BlockManager& blockman, const CBlockIndex* pblockindex)
{
CBlock block;
{
LOCK(cs_main);
if (blockman.IsBlockPruned(pblockindex)) {
if (pblockindex->nHeight == 0) {
return blockman.GetParams().GenesisBlock();
}
throw JSONRPCError(RP
...

💬 ajtowns commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1670579639)
I don't understand why we'd want to touch consensus code for something that's at most a slightly confusing error message? Just changing the error description as in #28234 seems a strictly better approach.

As it stands, `SCRIPT_ERR_DISABLED_OPCODE` covers opcodes that are unusable even in unexecuted IF/ELSE branches, while `SCRIPT_ERR_BAD_OPCODE` covers opcodes that are only unusable when evaluated.

💬 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