💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1944231008)
That's an absurd thing to do, so I'm worried that explaining it just makes the text hard to follow. That could miners to ignore the import warning about setting this value too low.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1944231008)
That's an absurd thing to do, so I'm worried that explaining it just makes the text hard to follow. That could miners to ignore the import warning about setting this value too low.
💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1944235910)
Hmm the getaddressinfo for this address displays it as false.
```
{'address': 'bcrt1q4dsf6kwgrraj6v6zvcr4a4cwlrfdvmnd0mh8yqcgpzrnryep5l5s4fr46d', 'scriptPubKey': '0020ab609d59c818fb2d334266075ed70ef8d2d66e6d7eee7203080887319321a7e9', 'ismine': True, 'solvable': True, 'desc': 'wsh(multi(1,[978e2eb7]0284a8013c99c26703031450d5dbed76aae4167e83241b43f9f12375ddedebcef8))#str4m9tu', 'parent_desc': 'wsh(multi(1,0284a8013c99c26703031450d5dbed76aae4167e83241b43f9f12375ddedebcef8))#qqala45t', 'iswatcho
...
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1944235910)
Hmm the getaddressinfo for this address displays it as false.
```
{'address': 'bcrt1q4dsf6kwgrraj6v6zvcr4a4cwlrfdvmnd0mh8yqcgpzrnryep5l5s4fr46d', 'scriptPubKey': '0020ab609d59c818fb2d334266075ed70ef8d2d66e6d7eee7203080887319321a7e9', 'ismine': True, 'solvable': True, 'desc': 'wsh(multi(1,[978e2eb7]0284a8013c99c26703031450d5dbed76aae4167e83241b43f9f12375ddedebcef8))#str4m9tu', 'parent_desc': 'wsh(multi(1,0284a8013c99c26703031450d5dbed76aae4167e83241b43f9f12375ddedebcef8))#qqala45t', 'iswatcho
...
💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1944239980)
I'm not sure how `iswatchonly` is supposed to behave in watch-only _descriptor_ wallets. cc @achow101
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1944239980)
I'm not sure how `iswatchonly` is supposed to behave in watch-only _descriptor_ wallets. cc @achow101
💬 maflcko commented on pull request "wallet, rpc: deprecate settxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2639084458)
Thinking more about the motivation, I agree with it, but I fail to see the connection to removing this RPC:
* The RPC doesn't rule out dynamic fee estimation. In fact, 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 wants to shoot themselves into the foot by setting the feerate to a constant
...
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2639084458)
Thinking more about the motivation, I agree with it, but I fail to see the connection to removing this RPC:
* The RPC doesn't rule out dynamic fee estimation. In fact, 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 wants to shoot themselves into the foot by setting the feerate to a constant
...
💬 pythcoiner commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1944282152)
done
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1944282152)
done
💬 pythcoiner commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1944282491)
done
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1944282491)
done
💬 maflcko commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2639134040)
I wonder if there is a way to detect those at compile or link time. Other than that, printing a warning when mixing two compilers when compiling with depends may be useful?
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2639134040)
I wonder if there is a way to detect those at compile or link time. Other than that, printing a warning when mixing two compilers when compiling with depends may be useful?
💬 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.