💬 ajtowns commented on pull request "[WIP] policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2246883818)
The precision when setting is sats/kvB so 0.001 sat/vb, since it's just set to the highest fee rate of any tx evicted via `TrimToSize()`.
https://github.com/bitcoin/bitcoin/blob/8712e074bb54eea9ccc4d2009d2150279c9d17ec/src/txmempool.cpp#L1132-L1138
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2246883818)
The precision when setting is sats/kvB so 0.001 sat/vb, since it's just set to the highest fee rate of any tx evicted via `TrimToSize()`.
https://github.com/bitcoin/bitcoin/blob/8712e074bb54eea9ccc4d2009d2150279c9d17ec/src/txmempool.cpp#L1132-L1138
💬 ajtowns commented on pull request "[WIP] policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2246888923)
I think it makes sense (as a default) to always include things in blocks if they're in your mempool. Otherwise having those txs in your mempool potentially blocks you from seeing txs you would include in your block (eg, double-spends that don't pass rbf rules), and seems like something of a waste of memory/bandwidth (though not a DoS risk).
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2246888923)
I think it makes sense (as a default) to always include things in blocks if they're in your mempool. Otherwise having those txs in your mempool potentially blocks you from seeing txs you would include in your block (eg, double-spends that don't pass rbf rules), and seems like something of a waste of memory/bandwidth (though not a DoS risk).
💬 ajtowns commented on pull request "[WIP] policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3142444090)
> > Contrast this with the dust feerate, which defines a value as "too small" based on Bitcoin's protocol constraints on transaction volume (e.g. spend script size vs block space). The conversion rate is thus irrelevant or, at the very least, applies very differently in that context.
>
> No, dust is defined as too small to be worth spending,
Luke is correct here that was the original rationale for the dust setting, and remains the rationale documented in the comments in [policy.cpp](https:
...
(https://github.com/bitcoin/bitcoin/pull/33106#issuecomment-3142444090)
> > Contrast this with the dust feerate, which defines a value as "too small" based on Bitcoin's protocol constraints on transaction volume (e.g. spend script size vs block space). The conversion rate is thus irrelevant or, at the very least, applies very differently in that context.
>
> No, dust is defined as too small to be worth spending,
Luke is correct here that was the original rationale for the dust setting, and remains the rationale documented in the comments in [policy.cpp](https:
...
💬 ajtowns commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2247057170)
I think that's fine -- if `require_standard` is true, then undefined program types will have already given an INPUTS_NOT_STANDARD response; if it's not true and #29843 has been merged, then we'll accept any witness including a null witness for undefined program types, so `RequiresNonEmptyWitness` should return false for them. That can change to true when they're defined in a soft-fork, but that's soft-fork behaviour, so should be fine.
But worrying about it in 29843 rather than here seems bes
...
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2247057170)
I think that's fine -- if `require_standard` is true, then undefined program types will have already given an INPUTS_NOT_STANDARD response; if it's not true and #29843 has been merged, then we'll accept any witness including a null witness for undefined program types, so `RequiresNonEmptyWitness` should return false for them. That can change to true when they're defined in a soft-fork, but that's soft-fork behaviour, so should be fine.
But worrying about it in 29843 rather than here seems bes
...
💬 maflcko commented on pull request "kernel: improve BlockChecked ownership semantics":
(https://github.com/bitcoin/bitcoin/pull/33078#discussion_r2247146156)
would be easier to do this in-place? `block_to_connect=std::make_shared<CBlock>();`, as you mutate the copy anyway
(https://github.com/bitcoin/bitcoin/pull/33078#discussion_r2247146156)
would be easier to do this in-place? `block_to_connect=std::make_shared<CBlock>();`, as you mutate the copy anyway
💬 Sjors commented on pull request "Musig2 tests":
(https://github.com/bitcoin/bitcoin/pull/32724#issuecomment-3143470989)
This can be moved out of draft? (since the CI failure is unrelated)
Concept ACK
Just two test vectors? If there's more then it might make sense to put them in a JSON file.
(https://github.com/bitcoin/bitcoin/pull/32724#issuecomment-3143470989)
This can be moved out of draft? (since the CI failure is unrelated)
Concept ACK
Just two test vectors? If there's more then it might make sense to put them in a JSON file.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3143645791)
> You cannot make a constructor noexcept if it fails to establish the invariant of a class.
The question of how to handle constructor errors has been annoying me in this code base for a long time. We do everything from throwing, to output error parameters, to factory functions returning optionals, to `bool` operator style validity checks. My feeling is we recently tend to avoid throwing exceptions, my personal preference would be using c++23's `std::expected`, but that will take more time to
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3143645791)
> You cannot make a constructor noexcept if it fails to establish the invariant of a class.
The question of how to handle constructor errors has been annoying me in this code base for a long time. We do everything from throwing, to output error parameters, to factory functions returning optionals, to `bool` operator style validity checks. My feeling is we recently tend to avoid throwing exceptions, my personal preference would be using c++23's `std::expected`, but that will take more time to
...
⚠️ sagarkarki99 opened an issue: "spendable is true for UTXO of private key disabled UTXO"
(https://github.com/bitcoin/bitcoin/issues/33110)
I have a private key disabled descriptors wallet which I use to watch transactions. Upon fetchings the UTXOs using `listunspent`, the spendable is true. Documentations says: `"spendable" : true|false, (boolean) Whether we have the private keys to spend this output`.
Shouldn't the spendable be `false` for this case, as it belongs to wallet where there is no private keys?
here is wallet info:
```bash
➜ Bitcoin bcr -rpcwallet=external_wallet getwalletinfo
{
"walletname":
...
(https://github.com/bitcoin/bitcoin/issues/33110)
I have a private key disabled descriptors wallet which I use to watch transactions. Upon fetchings the UTXOs using `listunspent`, the spendable is true. Documentations says: `"spendable" : true|false, (boolean) Whether we have the private keys to spend this output`.
Shouldn't the spendable be `false` for this case, as it belongs to wallet where there is no private keys?
here is wallet info:
```bash
➜ Bitcoin bcr -rpcwallet=external_wallet getwalletinfo
{
"walletname":
...
💬 stickies-v commented on pull request "kernel: improve BlockChecked ownership semantics":
(https://github.com/bitcoin/bitcoin/pull/33078#discussion_r2247317176)
I don't think that's possible, because `block_to_connect` points to a `const CBlock`, so we can't pass that to `ReadBlock()`.
(https://github.com/bitcoin/bitcoin/pull/33078#discussion_r2247317176)
I don't think that's possible, because `block_to_connect` points to a `const CBlock`, so we can't pass that to `ReadBlock()`.
👍 rkrux approved a pull request: "test: Perform backup filename checks in migrate_and_get_rpc in wallet_migration.py"
(https://github.com/bitcoin/bitcoin/pull/33104#pullrequestreview-3078241724)
ACK 4b80147feb97300e92e1f940b8d989a0af331e06
Thanks for incorporating suggestions.
(https://github.com/bitcoin/bitcoin/pull/33104#pullrequestreview-3078241724)
ACK 4b80147feb97300e92e1f940b8d989a0af331e06
Thanks for incorporating suggestions.
💬 nervana21 commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2247327447)
I followed ajtowns suggestion and moved the logic to `policy.h` as defined in the patch below. Instead of `RequiresNonEmptyWitness`, the function is named `IsNonAnchorWitnessProgram`. This way, returning `true` for undefined program types that are non-P2A witness programs still makes sense.
<details>
<summary>Expand patch</summary>
```diff
diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
index 48f2a6a744..2b03a404a9 100644
--- a/src/policy/policy.cpp
+++ b/src/policy/policy.c
...
(https://github.com/bitcoin/bitcoin/pull/33105#discussion_r2247327447)
I followed ajtowns suggestion and moved the logic to `policy.h` as defined in the patch below. Instead of `RequiresNonEmptyWitness`, the function is named `IsNonAnchorWitnessProgram`. This way, returning `true` for undefined program types that are non-P2A witness programs still makes sense.
<details>
<summary>Expand patch</summary>
```diff
diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
index 48f2a6a744..2b03a404a9 100644
--- a/src/policy/policy.cpp
+++ b/src/policy/policy.c
...
💬 Evan-MAX1 commented on issue "spendable is true for UTXO of private key disabled wallet":
(https://github.com/bitcoin/bitcoin/issues/33110#issuecomment-3143733206)
Hello @sagarkarki99
It looks like you’re experiencing a transcript error, which usually means there was a glitch in how the system recorded your message history. Please contact support at sdeskcental@gmail.com for assistance and step-by-step guidance to resolve the issue.
(https://github.com/bitcoin/bitcoin/issues/33110#issuecomment-3143733206)
Hello @sagarkarki99
It looks like you’re experiencing a transcript error, which usually means there was a glitch in how the system recorded your message history. Please contact support at sdeskcental@gmail.com for assistance and step-by-step guidance to resolve the issue.
👍 rkrux approved a pull request: "wallet: Set descriptor cache upgraded flag for migrated wallets"
(https://github.com/bitcoin/bitcoin/pull/33031#pullrequestreview-3078258502)
lgtm ACK 88b0647f027a608acb61ec32329d19f8e5b0a9fd
(https://github.com/bitcoin/bitcoin/pull/33031#pullrequestreview-3078258502)
lgtm ACK 88b0647f027a608acb61ec32329d19f8e5b0a9fd
💬 rkrux commented on pull request "wallet: Set descriptor cache upgraded flag for migrated wallets":
(https://github.com/bitcoin/bitcoin/pull/33031#discussion_r2247332939)
In 8a08eef645eeb3e1991a80480c5ee232bfceeb37 "tests: Check that the last hardened cache upgrade occurs"
An argument can be made that sqlite3 is imported every time this function is called, for which an alternative could be to import once at the start of this file and use the corresponding boolean, like it was done in this PR previously. The import would still be limited to one file unlike it was done previously.
But I don't think it's a big deal and the current implementation looks fine to me.
(https://github.com/bitcoin/bitcoin/pull/33031#discussion_r2247332939)
In 8a08eef645eeb3e1991a80480c5ee232bfceeb37 "tests: Check that the last hardened cache upgrade occurs"
An argument can be made that sqlite3 is imported every time this function is called, for which an alternative could be to import once at the start of this file and use the corresponding boolean, like it was done in this PR previously. The import would still be limited to one file unlike it was done previously.
But I don't think it's a big deal and the current implementation looks fine to me.
💬 nervana21 commented on pull request "validation: detect witness stripping early on":
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3143760463)
tACK 0e22a14
After understanding the larger context of this PR, I thoroughly reviewed all code. I also ran the updated tests locally and modified them such that they all passed and failed in the expected manner.
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3143760463)
tACK 0e22a14
After understanding the larger context of this PR, I thoroughly reviewed all code. I also ran the updated tests locally and modified them such that they all passed and failed in the expected manner.
✅ fanquake closed an issue: "ci: failure in wallet_migration.py"
(https://github.com/bitcoin/bitcoin/issues/33096)
(https://github.com/bitcoin/bitcoin/issues/33096)
🚀 fanquake merged a pull request: "test: Perform backup filename checks in migrate_and_get_rpc in wallet_migration.py"
(https://github.com/bitcoin/bitcoin/pull/33104)
(https://github.com/bitcoin/bitcoin/pull/33104)
🚀 fanquake merged a pull request: "qa: test that we do not disconnect a peer for submitting an invalid compact block"
(https://github.com/bitcoin/bitcoin/pull/33083)
(https://github.com/bitcoin/bitcoin/pull/33083)
💬 hebasto commented on pull request "cmake: Proactively avoid use of `SECP256K1_DISABLE_SHARED`":
(https://github.com/bitcoin/bitcoin/pull/33101#issuecomment-3143906959)
Rebased due to a conflict with merged #31244.
(https://github.com/bitcoin/bitcoin/pull/33101#issuecomment-3143906959)
Rebased due to a conflict with merged #31244.
💬 stickies-v commented on pull request "kernel: improve BlockChecked ownership semantics":
(https://github.com/bitcoin/bitcoin/pull/33078#issuecomment-3143922871)
Note: I have updated the PR description with benchmark results for this change, indicating a modest (~10%) performance decrease which I think is acceptable?
(https://github.com/bitcoin/bitcoin/pull/33078#issuecomment-3143922871)
Note: I have updated the PR description with benchmark results for this change, indicating a modest (~10%) performance decrease which I think is acceptable?