💬 optout21 commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514301870)
As a progression towards `getCoinbase` & deprecation, this implementation could internally also do what is suggested in the deprecation node:
invoke `ExtractCoinbaseTemplate`, scan outputs for the SegWit marker, and return the index.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514301870)
As a progression towards `getCoinbase` & deprecation, this implementation could internally also do what is suggested in the deprecation node:
invoke `ExtractCoinbaseTemplate`, scan outputs for the SegWit marker, and return the index.
💬 optout21 commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3517016509)
Concept ACK
The added new method makes sense. The deprecating methods are only being marked in the header comments, they are kept, there is no formal plan for deprecation. But there is a fair plan for switching users, and keeping them is not an extra burden.
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3517016509)
Concept ACK
The added new method makes sense. The deprecating methods are only being marked in the header comments, they are kept, there is no formal plan for deprecation. But there is a fair plan for switching users, and keeping them is not an extra burden.
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514303921)
This returns the full `scriptPubKey` as generated by `GenerateCoinbaseCommitment`. Although you can derive that from the `witness` field, I don't think one should.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514303921)
This returns the full `scriptPubKey` as generated by `GenerateCoinbaseCommitment`. Although you can derive that from the `witness` field, I don't think one should.
🚀 fanquake merged a pull request: " test: [refactor] Use reference over ptr to chainman "
(https://github.com/bitcoin/bitcoin/pull/33840)
(https://github.com/bitcoin/bitcoin/pull/33840)
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514313165)
Let's also wait and see what comes out of https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514193806 because if I have to modify the signature of `submitSolution()` (to return a `CBlock`), that's the kind of breaking change I think is worth making.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514313165)
Let's also wait and see what comes out of https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514193806 because if I have to modify the signature of `submitSolution()` (to return a `CBlock`), that's the kind of breaking change I think is worth making.
💬 optout21 commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514314492)
My reasoning was to add extra assurance that the deprecated and new method gives the same result, it's a simple test. But it's fine as it is.
(https://github.com/bitcoin/bitcoin/pull/33819#discussion_r2514314492)
My reasoning was to add extra assurance that the deprecated and new method gives the same result, it's a simple test. But it's fine as it is.
💬 TheCharlatan commented on pull request "kernel: Improve logging API":
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3517090033)
Concept ACK
Thank you for contributing this @ryanofsky! We've discussed doing something similar a few months ago. This is close to how I hoped to implement the logging capabilities, but reviewers highlighted some inconsistencies with this approach: A secondary logger object cannot receive its own non-global options. The docstrings do at least still explain this even with this change, I am not sure if it is really less surprising for users. Maybe it would be preferable to push forward #30342 b
...
(https://github.com/bitcoin/bitcoin/pull/33847#issuecomment-3517090033)
Concept ACK
Thank you for contributing this @ryanofsky! We've discussed doing something similar a few months ago. This is close to how I hoped to implement the logging capabilities, but reviewers highlighted some inconsistencies with this approach: A secondary logger object cannot receive its own non-global options. The docstrings do at least still explain this even with this change, I am not sure if it is really less surprising for users. Maybe it would be preferable to push forward #30342 b
...
💬 ismaelsadeeq commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514393534)
No need to revive https://github.com/bitcoin/bitcoin/pull/33374 I saw it yesterday and will push an update here soon.
We already made a copy of the block here, we should copy the whole template, it is a cheap copy I think.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2514393534)
No need to revive https://github.com/bitcoin/bitcoin/pull/33374 I saw it yesterday and will push an update here soon.
We already made a copy of the block here, we should copy the whole template, it is a cheap copy I think.
📝 Eunovo opened a pull request: "validation: fix assumevalid is ignored during reindex"
(https://github.com/bitcoin/bitcoin/pull/33854)
During a reindex, the assumevalid setting may be ignored for the following reasons:
- The assumevalid block is missing from the block index. This can occur if the block could not be downloaded before a previous IBD run was interrupted.
- The chainwork of the best header is below `minimumchainwork`. This happens when the previous IBD was stopped before connecting enough blocks to reach the required `minimumchainwork`. See [Issue #31494](https://github.com/bitcoin/bitcoin/issues/31494).
As a
...
(https://github.com/bitcoin/bitcoin/pull/33854)
During a reindex, the assumevalid setting may be ignored for the following reasons:
- The assumevalid block is missing from the block index. This can occur if the block could not be downloaded before a previous IBD run was interrupted.
- The chainwork of the best header is below `minimumchainwork`. This happens when the previous IBD was stopped before connecting enough blocks to reach the required `minimumchainwork`. See [Issue #31494](https://github.com/bitcoin/bitcoin/issues/31494).
As a
...
✅ Eunovo closed a pull request: "validation: ensure assumevalid is always used during reindex"
(https://github.com/bitcoin/bitcoin/pull/31615)
(https://github.com/bitcoin/bitcoin/pull/31615)
💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3517155840)
Thanks for the reviews and suggestions @l0rinc @stickies-v @hodlinator @mzumsande @TheCharlatan @stratospher .
I have implemented an approach that uses the headers-sync approach in https://github.com/bitcoin/bitcoin/pull/33854 and will be closing this PR.
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3517155840)
Thanks for the reviews and suggestions @l0rinc @stickies-v @hodlinator @mzumsande @TheCharlatan @stratospher .
I have implemented an approach that uses the headers-sync approach in https://github.com/bitcoin/bitcoin/pull/33854 and will be closing this PR.
💬 glozow commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3517174948)
> Returning feerates in BTC/kvB it can be very burdensome and is not good practice, as the most widely adopted units are sat/vB.
Can you explain why it's not good practice? I agree it's not as human-readable but I don't software minds either way. Not to be snarky, but I think the most widely adopted usage of this RPC interface must be to handle what it returns, including converting it before printing to users.
> This PR aims to show a PoC of how we could migrate to sat/vb in a backwards c
...
(https://github.com/bitcoin/bitcoin/pull/33741#issuecomment-3517174948)
> Returning feerates in BTC/kvB it can be very burdensome and is not good practice, as the most widely adopted units are sat/vB.
Can you explain why it's not good practice? I agree it's not as human-readable but I don't software minds either way. Not to be snarky, but I think the most widely adopted usage of this RPC interface must be to handle what it returns, including converting it before printing to users.
> This PR aims to show a PoC of how we could migrate to sat/vb in a backwards c
...
🤔 rkrux reviewed a pull request: "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors"
(https://github.com/bitcoin/bitcoin/pull/33392#pullrequestreview-3447945798)
Concept ACK and cursory review 1ed236e3c686582e35f20ab668cd6d93316fae2c.
The intent of the PR and the corresponding issue #28898 seems fine to me.
(https://github.com/bitcoin/bitcoin/pull/33392#pullrequestreview-3447945798)
Concept ACK and cursory review 1ed236e3c686582e35f20ab668cd6d93316fae2c.
The intent of the PR and the corresponding issue #28898 seems fine to me.
💬 rkrux commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514179835)
In https://github.com/bitcoin/bitcoin/commit/82a222b00d6497dc22e85205dae577a1adf15989 "rpc: extend importdescriptors with UTXO check and incremental rescan"
Why is the UTXO set balance check (and the corresponding incremental block scan incase of balance mismatch) done before the usual rescan that's supposed to be done upon a successful descriptor import? Should this not be done after the usual rescan to find any transactions in the older blocks that might have been missed in the rescan becau
...
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514179835)
In https://github.com/bitcoin/bitcoin/commit/82a222b00d6497dc22e85205dae577a1adf15989 "rpc: extend importdescriptors with UTXO check and incremental rescan"
Why is the UTXO set balance check (and the corresponding incremental block scan incase of balance mismatch) done before the usual rescan that's supposed to be done upon a successful descriptor import? Should this not be done after the usual rescan to find any transactions in the older blocks that might have been missed in the rescan becau
...
💬 rkrux commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514297779)
In https://github.com/bitcoin/bitcoin/commit/fa3ac7378ca016c598d51fa00a9cc02103754569 "wallet/rpc: add scan_utxoset arg & docs for importdescriptors"
This string property seems unnecessary if it was have only one value all the time.
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514297779)
In https://github.com/bitcoin/bitcoin/commit/fa3ac7378ca016c598d51fa00a9cc02103754569 "wallet/rpc: add scan_utxoset arg & docs for importdescriptors"
This string property seems unnecessary if it was have only one value all the time.
💬 rkrux commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514222126)
In https://github.com/bitcoin/bitcoin/commit/82a222b00d6497dc22e85205dae577a1adf15989 "rpc: extend importdescriptors with UTXO check and incremental rescan"
Shouldn't the timestamp of the imported descriptors be updated as well in this case to avoid data inconsistency?
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514222126)
In https://github.com/bitcoin/bitcoin/commit/82a222b00d6497dc22e85205dae577a1adf15989 "rpc: extend importdescriptors with UTXO check and incremental rescan"
Shouldn't the timestamp of the imported descriptors be updated as well in this case to avoid data inconsistency?
💬 rkrux commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514186801)
In fa3ac7378ca016c598d51fa00a9cc02103754569 "wallet/rpc: add scan_utxoset arg & docs for importdescriptors"
```
s/"scan_utxoset"/"verify"
```
From a user's POV, this feels to me more like a verification step that the wallet might do if set by the user. Let's just call it that to keep it simple for the user?
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514186801)
In fa3ac7378ca016c598d51fa00a9cc02103754569 "wallet/rpc: add scan_utxoset arg & docs for importdescriptors"
```
s/"scan_utxoset"/"verify"
```
From a user's POV, this feels to me more like a verification step that the wallet might do if set by the user. Let's just call it that to keep it simple for the user?
💬 rkrux commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514139728)
In 82a222b00d6497dc22e85205dae577a1adf15989 "rpc: extend importdescriptors with UTXO check and incremental rescan"
The lack of nesting in the `if` blocks makes the code harder to read.
```diff
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 4146a65876..cf8269b2ef 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -507,7 +507,6 @@ RPCHelpMan importdescriptors()
bool have_prune_boundary = false;
int64_t min_trial_start_time = 0;
...
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514139728)
In 82a222b00d6497dc22e85205dae577a1adf15989 "rpc: extend importdescriptors with UTXO check and incremental rescan"
The lack of nesting in the `if` blocks makes the code harder to read.
```diff
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 4146a65876..cf8269b2ef 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -507,7 +507,6 @@ RPCHelpMan importdescriptors()
bool have_prune_boundary = false;
int64_t min_trial_start_time = 0;
...
💬 rkrux commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514188570)
In 1ed236e3c686582e35f20ab668cd6d93316fae2c "test: add functional test for importdescriptors scan_utxo flag"
What's the reason to assert this at only this stage?
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514188570)
In 1ed236e3c686582e35f20ab668cd6d93316fae2c "test: add functional test for importdescriptors scan_utxo flag"
What's the reason to assert this at only this stage?
💬 rkrux commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514333367)
In https://github.com/bitcoin/bitcoin/commit/fa3ac7378ca016c598d51fa00a9cc02103754569 "wallet/rpc: add scan_utxoset arg & docs for importdescriptors"
I think these don't need to be marked optional when the `info` object is already marked so. Unless these properties appear optionally inside the `info` object that I think is not the case.
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2514333367)
In https://github.com/bitcoin/bitcoin/commit/fa3ac7378ca016c598d51fa00a9cc02103754569 "wallet/rpc: add scan_utxoset arg & docs for importdescriptors"
I think these don't need to be marked optional when the `info` object is already marked so. Unless these properties appear optionally inside the `info` object that I think is not the case.