💬 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?
(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
(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
...
(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?
(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
...
(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
(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?
(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 ||
...
(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.
(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?
(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
...
(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
(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.
(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
...
(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.
(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.
(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
(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.
(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)
(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)
(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)