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

💬 glozow commented on pull request "policy: nVersion=3 and Package RBF":
(https://github.com/bitcoin/bitcoin/pull/25038#discussion_r1288170497)
Thanks for catching, should be fixed now

💬 glozow commented on pull request "mempool / miner: regularly flush <=0-fee entries and mine everything in the mempool":
(https://github.com/bitcoin/bitcoin/pull/27018#issuecomment-1670959695)
Closing in favor of #27677 which would solve the more general issue of selection score != eviction score.

Noting that #25038 adds trimming of things below min relay feerate, but we only expect those transactions as a result of reorgs or replacement of v3 children. v3 doesn't have the the issue of selection score != eviction score due to topological restrictions.

✅ glozow closed a pull request: "mempool / miner: regularly flush <=0-fee entries and mine everything in the mempool"
(https://github.com/bitcoin/bitcoin/pull/27018)

📝 Sjors opened a pull request: "Silent payment index (for light wallets and consistency check)"
(https://github.com/bitcoin/bitcoin/pull/28241)
This PR adds an index with the silent payment tweak for every transaction. It builds on top of #28122.

Based on the index that was originally part of #24897.

_Not ready for review._

TODO:

- [ ] replace `SilentPaymentIndex::ExtractPubkeyFromInput` with a call to existing helper functions like `SumInputPubKeys`
- [ ] provide a way to dump the index, e.g. using a `getsilentpaymentblockdata` RPC like in 782d7eb215ee37049b636aa3dce82f16615312bd (but getting data from the index)

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1670965334)
I'd like to keep this based on #28122 but that requires some refactoring there. See https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1670943979.

💬 josibake commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-1670999533)
> I'd like to keep this based on #28122 but that requires some refactoring there. See [#28122 (comment)](https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1670943979).

you can also rebase on https://github.com/bitcoin/bitcoin/pull/27827 , which has everything, or just the receiving PR. Might be easier then to reason about which functions belong in which PRs if we can see it all together. I think the functions you need are introduced in the receiving PR (#28202)

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1288228607)
Removed mention of "modified," as it's not really relevant whether it's real fees or not

💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1288237266)
> That seems to be the case when opening this against my fork: [jarolrod#2](https://github.com/jarolrod/bitcoin/pull/2)

That's correct. But is this really the case we want to care about?

👍 dergoegge approved a pull request: "refactor: Remove unused boost signals2 from torcontrol"
(https://github.com/bitcoin/bitcoin/pull/28240#pullrequestreview-1569180518)
utACK faaba770e11352ddf6414b9855f4baa46a967580