💬 Sjors commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2639150752)
ACK 39e15f6ca4fd44cc459d80b7a1540c03923906dd
It's good to clarify the description, because the term `NODE_NETWORK_LIMITED` has confused me before into assuming that a node that isn't limited doesn't use the flag. But instead it signals the minimum capability, and `NODE_NETWORK` is added when ready to signal full capability. Too late to rename them.
You can also see this in the `init.cpp`.
We start with just `NODE_NETWORK_LIMITED`:
https://github.com/bitcoin/bitcoin/blob/82ba50513425b
...
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2639150752)
ACK 39e15f6ca4fd44cc459d80b7a1540c03923906dd
It's good to clarify the description, because the term `NODE_NETWORK_LIMITED` has confused me before into assuming that a node that isn't limited doesn't use the flag. But instead it signals the minimum capability, and `NODE_NETWORK` is added when ready to signal full capability. Too late to rename them.
You can also see this in the `init.cpp`.
We start with just `NODE_NETWORK_LIMITED`:
https://github.com/bitcoin/bitcoin/blob/82ba50513425b
...
💬 maflcko commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1944334633)
I understand that bip 159 uses the word "pruning" and that it can be used as an alias for "limited", but internally in Bitcoin Core, pruning usually means that blocks are deleted. However, a limited peer may be limited for other reasons, for example:
* AU-background download is still in progress (possible today in Bitcoin Core)
* A node that just wants to limit itself (similar to maxuploadtarget?)
* ...
So I think it would be better to not imply the two words mean the same here.
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1944334633)
I understand that bip 159 uses the word "pruning" and that it can be used as an alias for "limited", but internally in Bitcoin Core, pruning usually means that blocks are deleted. However, a limited peer may be limited for other reasons, for example:
* AU-background download is still in progress (possible today in Bitcoin Core)
* A node that just wants to limit itself (similar to maxuploadtarget?)
* ...
So I think it would be better to not imply the two words mean the same here.
💬 vasild commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2639207566)
`CNetAddr::GetBIP155Network()` might be useful.
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2639207566)
`CNetAddr::GetBIP155Network()` might be useful.
💬 Sjors commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2639237263)
Can you mention in the PR description that this builds on #31649? That also helps prevent review comments on the checkpoint change from being spread between two pull requests.
The first two `[refactor]` commits 0dad68ec06af4c53bf4a3cd6c266e8cb8fe5ef55 and 9655edaec317f0e8aa14ff7feab68ec668a86076 look correct to me.
Although the `CheckHeaderNotFuture` is now performed earlier than before, but it's cheap enough that this doesn't seem dangerous. The tests that it moves ahead of are similarly
...
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2639237263)
Can you mention in the PR description that this builds on #31649? That also helps prevent review comments on the checkpoint change from being spread between two pull requests.
The first two `[refactor]` commits 0dad68ec06af4c53bf4a3cd6c266e8cb8fe5ef55 and 9655edaec317f0e8aa14ff7feab68ec668a86076 look correct to me.
Although the `CheckHeaderNotFuture` is now performed earlier than before, but it's cheap enough that this doesn't seem dangerous. The tests that it moves ahead of are similarly
...
💬 polespinasa commented on pull request "wallet, rpc: deprecate settxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2639288816)
> * I expect some users not using Bitcoin Core's fee estimation, but an external one. This means that the fee estimates will need to be fed to Bitcoin Core. One way to do it is via the global RPC, another one is via the per-transaction RPC(s).
If a user relies on an external fee estimation service, setting a static wallet-wide fee rate makes little sense. Fees are dynamic, and the best practice is to set them individually per transaction rather than globally regardless of what you use to esti
...
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2639288816)
> * I expect some users not using Bitcoin Core's fee estimation, but an external one. This means that the fee estimates will need to be fed to Bitcoin Core. One way to do it is via the global RPC, another one is via the per-transaction RPC(s).
If a user relies on an external fee estimation service, setting a static wallet-wide fee rate makes little sense. Fees are dynamic, and the best practice is to set them individually per transaction rather than globally regardless of what you use to esti
...
💬 Sjors commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1944416573)
Someone else making an invalid block is not a reason for our node to shut down, so `LogError` would be wrong.
We also run this before the actual proof-of-work check, and headers can be sent repeatedly, so need to be careful about log spam.
You can however drop `__func__`, because we have `-logsourcelocations`.
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1944416573)
Someone else making an invalid block is not a reason for our node to shut down, so `LogError` would be wrong.
We also run this before the actual proof-of-work check, and headers can be sent repeatedly, so need to be careful about log spam.
You can however drop `__func__`, because we have `-logsourcelocations`.
💬 Sjors commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1944435217)
9655edaec317f0e8aa14ff7feab68ec668a86076 nit: you can drop __func__.
The use of `LogError` here is fine; it's consistent with the other checks, and this code isn't accessible via p2p.
(https://github.com/bitcoin/bitcoin/pull/31792#discussion_r1944435217)
9655edaec317f0e8aa14ff7feab68ec668a86076 nit: you can drop __func__.
The use of `LogError` here is fine; it's consistent with the other checks, and this code isn't accessible via p2p.
🤔 rkrux reviewed a pull request: "Enhanced error messages for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260#pullrequestreview-2598102040)
I have reviewed the first 2 commits only as I found the 3rd commit to be difficult to review. I agree with the idea of splitting the last commit into multiple commits as mentioned previously: https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2390929867.
Overall I agree with the idea of improving the error messages when the address decoding fails but I don't think I agree with the below statement because from the POV of the network that address in indeed invalid. Ref: https://en.bitco
...
(https://github.com/bitcoin/bitcoin/pull/27260#pullrequestreview-2598102040)
I have reviewed the first 2 commits only as I found the 3rd commit to be difficult to review. I agree with the idea of splitting the last commit into multiple commits as mentioned previously: https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2390929867.
Overall I agree with the idea of improving the error messages when the address decoding fails but I don't think I agree with the below statement because from the POV of the network that address in indeed invalid. Ref: https://en.bitco
...
💬 rkrux commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1944380861)
Nit: I found these multiple spaces after the `=` a little unusual (first time seeing this), imo it's fine to use just one.
```diff
- base58EncodedPrefixes[SECRET_KEY] = {"9","c"};
+ base58EncodedPrefixes[SECRET_KEY] = {"9","c"};
```
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1944380861)
Nit: I found these multiple spaces after the `=` a little unusual (first time seeing this), imo it's fine to use just one.
```diff
- base58EncodedPrefixes[SECRET_KEY] = {"9","c"};
+ base58EncodedPrefixes[SECRET_KEY] = {"9","c"};
```
💬 rkrux commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1944379865)
Confirming - Is using sentence case for mainnet correct? Rest of them use lower case.
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1944379865)
Confirming - Is using sentence case for mainnet correct? Rest of them use lower case.
💬 rkrux commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1944386520)
Super nit: Personal preference as I find it congruent with the English language and it's slightly easier on the eyes. Same for all these other newly added constants.
```diff
- base58EncodedPrefixes[PUBKEY_ADDRESS] = {"m","n"};
+ base58EncodedPrefixes[PUBKEY_ADDRESS] = {"m", "n"};
```
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1944386520)
Super nit: Personal preference as I find it congruent with the English language and it's slightly easier on the eyes. Same for all these other newly added constants.
```diff
- base58EncodedPrefixes[PUBKEY_ADDRESS] = {"m","n"};
+ base58EncodedPrefixes[PUBKEY_ADDRESS] = {"m", "n"};
```
🚀 fanquake merged a pull request: "ci: Remove no longer needed `-Wno-error=documentation`"
(https://github.com/bitcoin/bitcoin/pull/31804)
(https://github.com/bitcoin/bitcoin/pull/31804)
💬 Sjors commented on pull request "Add checkblock RPC and checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31564#discussion_r1944441787)
Note to self: to maintain existing behavior this should emit `LogError()`. That's not necessary for `generateblock` since it already throws.
Emitting `LogError()` in TestBlockValidity would be wrong as of this PR, because that function doesn't know if we're checking our own blocks (which would indicate a fatal problem) or someone else's.
(https://github.com/bitcoin/bitcoin/pull/31564#discussion_r1944441787)
Note to self: to maintain existing behavior this should emit `LogError()`. That's not necessary for `generateblock` since it already throws.
Emitting `LogError()` in TestBlockValidity would be wrong as of this PR, because that function doesn't know if we're checking our own blocks (which would indicate a fatal problem) or someone else's.
💬 maflcko commented on pull request "wallet, rpc: deprecate settxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2639357946)
> > * I expect some users not using Bitcoin Core's fee estimation, but an external one. This means that the fee estimates will need to be fed to Bitcoin Core. One way to do it is via the global RPC, another one is via the per-transaction RPC(s).
>
> If a user relies on an external fee estimation service, setting a static wallet-wide fee rate makes little sense. Fees are dynamic
I understand that fees are dynamic. But it seems reasonable to (let's say) call `settxfee` every minute to update
...
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2639357946)
> > * I expect some users not using Bitcoin Core's fee estimation, but an external one. This means that the fee estimates will need to be fed to Bitcoin Core. One way to do it is via the global RPC, another one is via the per-transaction RPC(s).
>
> If a user relies on an external fee estimation service, setting a static wallet-wide fee rate makes little sense. Fees are dynamic
I understand that fees are dynamic. But it seems reasonable to (let's say) call `settxfee` every minute to update
...
💬 maflcko commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#issuecomment-2639383254)
On a second thought, the GUI uses the derivative (progress increase per hour), so whenever progress decreases, the GUI will display a negative increase. Then, likely the same people that complain about the progress not being 1 will complain about the increase being negative (or zero).
I guess this scenario can already happen post-minchainwork-ibd, but it will probably happen more often when progress is just based on available headers, given that it is faster to download headers than to verify
...
(https://github.com/bitcoin/bitcoin/pull/31177#issuecomment-2639383254)
On a second thought, the GUI uses the derivative (progress increase per hour), so whenever progress decreases, the GUI will display a negative increase. Then, likely the same people that complain about the progress not being 1 will complain about the increase being negative (or zero).
I guess this scenario can already happen post-minchainwork-ibd, but it will probably happen more often when progress is just based on available headers, given that it is faster to download headers than to verify
...
📝 hebasto opened a pull request: "Prepare "Open Transifex translations for v29.0" release step"
(https://github.com/bitcoin/bitcoin/pull/31809)
This PR follows our [Release Process](https://github.com/bitcoin/bitcoin/blob/864386a7444fb5cf16613956ce8bf335f51b67d5/doc/release-process.md).
It is required to open Transifex translations for v29.0, as scheduled in https://github.com/bitcoin/bitcoin/issues/31029.
The previous similar PR: https://github.com/bitcoin/bitcoin/pull/30548.
**Notes for reviewers:**
1. This is the first release process conducted after migrating the build system to CMake. This revealed a bug, which is fixed
...
(https://github.com/bitcoin/bitcoin/pull/31809)
This PR follows our [Release Process](https://github.com/bitcoin/bitcoin/blob/864386a7444fb5cf16613956ce8bf335f51b67d5/doc/release-process.md).
It is required to open Transifex translations for v29.0, as scheduled in https://github.com/bitcoin/bitcoin/issues/31029.
The previous similar PR: https://github.com/bitcoin/bitcoin/pull/30548.
**Notes for reviewers:**
1. This is the first release process conducted after migrating the build system to CMake. This revealed a bug, which is fixed
...
💬 rkrux commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#issuecomment-2639461217)
Definite Concept ACK
This one is a biggie, I should get around to reviewing this PR sometime later this month and will leave a review then.
(https://github.com/bitcoin/bitcoin/pull/21283#issuecomment-2639461217)
Definite Concept ACK
This one is a biggie, I should get around to reviewing this PR sometime later this month and will leave a review then.
💬 hebasto commented on pull request "Prepare "Open Transifex translations for v29.0" release step":
(https://github.com/bitcoin/bitcoin/pull/31809#issuecomment-2639472097)
cc @stickies-v @pablomartin4btc @johnny9 @jarolrod as regular reviewers of similar previous PRs.
cc @l0rinc as the author of https://github.com/bitcoin/bitcoin/pull/31731.
cc @glozow as the 29.0 release manager.
(https://github.com/bitcoin/bitcoin/pull/31809#issuecomment-2639472097)
cc @stickies-v @pablomartin4btc @johnny9 @jarolrod as regular reviewers of similar previous PRs.
cc @l0rinc as the author of https://github.com/bitcoin/bitcoin/pull/31731.
cc @glozow as the 29.0 release manager.
💬 hebasto commented on pull request "doc: update translation generation cmake example":
(https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1944521310)
You might consider referencing https://github.com/bitcoin/bitcoin/pull/31809 for the actual process of regenerating a translation source file.
Another important point to amend in this doc is that the `translate` target generates the `src/qt/locale/bitcoin_en.xlf` file, which is the translation source for the Transifex:https://github.com/bitcoin/bitcoin/blob/d6c229d8bd4a6203a7255c140aa35c59fb20378b/.tx/config#L6
(https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1944521310)
You might consider referencing https://github.com/bitcoin/bitcoin/pull/31809 for the actual process of regenerating a translation source file.
Another important point to amend in this doc is that the `translate` target generates the `src/qt/locale/bitcoin_en.xlf` file, which is the translation source for the Transifex:https://github.com/bitcoin/bitcoin/blob/d6c229d8bd4a6203a7255c140aa35c59fb20378b/.tx/config#L6
💬 polespinasa commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#issuecomment-2639568292)
I'm not sure I understand your proposal correctly. You're proposing this offset on the original function version or the "new" one?
Would it get to 1.0 if we set a constant offset on all blocks?
I think this idea is kinda similar to this one: https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1943113886 but in this case I was thinking of applying it when the block time is far behind the local time.
(https://github.com/bitcoin/bitcoin/pull/31177#issuecomment-2639568292)
I'm not sure I understand your proposal correctly. You're proposing this offset on the original function version or the "new" one?
Would it get to 1.0 if we set a constant offset on all blocks?
I think this idea is kinda similar to this one: https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1943113886 but in this case I was thinking of applying it when the block time is far behind the local time.