🤔 jonatack reviewed a pull request: "cli: improve error message on multiwallet and add validation to cli-side commands"
(https://github.com/bitcoin/bitcoin/pull/26990#pullrequestreview-1860423961)
Reviewed the first commit fa48d460334beef43fa73455aa9e30971d33c1b2. It looks like the prefix in its commit name should be `cli` instead of `gui`. Will review the second commit later today or tomorrow.
<details><summary>combined suggested diff for <code>fa48d46</code></summary><p>
```diff
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index a53c2a1b0d0..b342382fc96 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -53,6 +53,7 @@ using CliClock = std::chrono::system_
...
(https://github.com/bitcoin/bitcoin/pull/26990#pullrequestreview-1860423961)
Reviewed the first commit fa48d460334beef43fa73455aa9e30971d33c1b2. It looks like the prefix in its commit name should be `cli` instead of `gui`. Will review the second commit later today or tomorrow.
<details><summary>combined suggested diff for <code>fa48d46</code></summary><p>
```diff
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index a53c2a1b0d0..b342382fc96 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -53,6 +53,7 @@ using CliClock = std::chrono::system_
...
💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476756492)
https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 `continue` here would be more idiomatic C++ and avoid indenting the next 30 lines.
```suggestion
if (!IsSwitchChar(argv[i][0])) continue;
```
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476756492)
https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 `continue` here would be more idiomatic C++ and avoid indenting the next 30 lines.
```suggestion
if (!IsSwitchChar(argv[i][0])) continue;
```
💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476751537)
fa48d460334beef43fa73455aa9e30971d33c1b2 This would output `error: Error:` and the duplicate element would be the bitcoin-cli command.
```suggestion
throw std::runtime_error(strprintf("duplicate bitcoin-cli command %s", command));
```
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476751537)
fa48d460334beef43fa73455aa9e30971d33c1b2 This would output `error: Error:` and the duplicate element would be the bitcoin-cli command.
```suggestion
throw std::runtime_error(strprintf("duplicate bitcoin-cli command %s", command));
```
💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476753642)
fa48d460334beef43fa73455aa9e30971d33c1b2 This would output `error: Error:` and would be clearer as `bitcoin-cli command`.
```suggestion
throw std::runtime_error(strprintf("invalid bitcoin-cli argument; if \"%s\" is a valid option, try passing it before the \"%s\" command", argv[i], exclusively_command));
```
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476753642)
fa48d460334beef43fa73455aa9e30971d33c1b2 This would output `error: Error:` and would be clearer as `bitcoin-cli command`.
```suggestion
throw std::runtime_error(strprintf("invalid bitcoin-cli argument; if \"%s\" is a valid option, try passing it before the \"%s\" command", argv[i], exclusively_command));
```
💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476759727)
https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 These two lines could be reduced to the following to keep `pos` scoped to the conditional only (and use `string_view::npos`).
```diff
- std::size_t pos = command.find('=');
- if (pos != std::string::npos) {
+ if (const auto pos{command.find_first_of('=')}; pos != std::string_view::npos) {
command = command.substr(0, pos);
}
```
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476759727)
https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 These two lines could be reduced to the following to keep `pos` scoped to the conditional only (and use `string_view::npos`).
```diff
- std::size_t pos = command.find('=');
- if (pos != std::string::npos) {
+ if (const auto pos{command.find_first_of('=')}; pos != std::string_view::npos) {
command = command.substr(0, pos);
}
```
💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476781372)
https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 This seems like a lot of code for what could be 2 lines instead, and you could make the commands data structure an `constexpr` array constant and have it run in O(n) instead of O(log n) (and order the commands by probable frequency of invocation). The resulting code is self-explanatory, so could drop the comments too.
<details><summary>Suggested diff</summary><p>
```diff
@@ -53,6 +53,7 @@ using CliClock = s
...
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476781372)
https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 This seems like a lot of code for what could be 2 lines instead, and you could make the commands data structure an `constexpr` array constant and have it run in O(n) instead of O(log n) (and order the commands by probable frequency of invocation). The resulting code is self-explanatory, so could drop the comments too.
<details><summary>Suggested diff</summary><p>
```diff
@@ -53,6 +53,7 @@ using CliClock = s
...
💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476783931)
https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 could do the cheaper check first
```suggestion
if (!exclusively_command.empty() && !gArgs.IsArgSet(std::string(command))) {
```
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476783931)
https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 could do the cheaper check first
```suggestion
if (!exclusively_command.empty() && !gArgs.IsArgSet(std::string(command))) {
```
💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476752815)
fa48d460334beef43fa73455aa9e30971d33c1b2 This would output `error: Error:` and would be clearer as `bitcoin-cli command`.
```suggestion
throw std::runtime_error(strprintf("you can only run one bitcoin-cli command at a time, either %s or %s", exclusively_command, command));
```
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476752815)
fa48d460334beef43fa73455aa9e30971d33c1b2 This would output `error: Error:` and would be clearer as `bitcoin-cli command`.
```suggestion
throw std::runtime_error(strprintf("you can only run one bitcoin-cli command at a time, either %s or %s", exclusively_command, command));
```
💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476755003)
https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 Can now use C++20 `std::set#contains` method.
```suggestion
if (commands.contains(command)) {
```
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476755003)
https://github.com/bitcoin/bitcoin/commit/fa48d460334beef43fa73455aa9e30971d33c1b2 Can now use C++20 `std::set#contains` method.
```suggestion
if (commands.contains(command)) {
```
💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1476795724)
If we get the same txid but different wtxid, I'm not sure that the result is necessarily one that makes sense. Most things in the wallet are keyed and listed by txid, so with this change, we'd end up with a transaction that's listed as conflicted by it's own txid. Also, we won't ever update the wallet to store a transaction with a different wtxid.
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1476795724)
If we get the same txid but different wtxid, I'm not sure that the result is necessarily one that makes sense. Most things in the wallet are keyed and listed by txid, so with this change, we'd end up with a transaction that's listed as conflicted by it's own txid. Also, we won't ever update the wallet to store a transaction with a different wtxid.
💬 jonatack commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476798503)
Using an unordered set instead of a set, `find`/`contains` complexity can be reduced from O(log n) to O(1). Remember to add the `#include <unordered_set>` header in this case.
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1476798503)
Using an unordered set instead of a set, `find`/`contains` complexity can be reduced from O(log n) to O(1). Remember to add the `#include <unordered_set>` header in this case.
⚠️ nsvrn opened an issue: "IBD stalling issue in v26.0 and assumevalid=0"
(https://github.com/bitcoin/bitcoin/issues/29374)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
I'm using `v26` with `assumevalid=0` and the IBD keeps stalling after the log message says `Potential stale tip detected, will try using extra outbound peer`, it retries but same message keeps repeating unless I restart the software.
I saw someone on bitcoin stack exchange suggested to not use VPN, I'm not using any VPN or tor. Just simple clearnet on a fresh Linux OS.
### Expected behav
...
(https://github.com/bitcoin/bitcoin/issues/29374)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
I'm using `v26` with `assumevalid=0` and the IBD keeps stalling after the log message says `Potential stale tip detected, will try using extra outbound peer`, it retries but same message keeps repeating unless I restart the software.
I saw someone on bitcoin stack exchange suggested to not use VPN, I'm not using any VPN or tor. Just simple clearnet on a fresh Linux OS.
### Expected behav
...
💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1476798246)
I think this hasn't been reverted yet?
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1476798246)
I think this hasn't been reverted yet?
💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1476797365)
In 5fbf18b89d6768bed1be95dcbc98797df5788dfc "wallet: track mempool conflicts"
I'm a little bit confused by the "erase this wtx entry" part of the comment. AFAICT, no entries are being erased, Perhaps this should say:
```suggestion
// If this wtx has no other mempool conflicts, mark it as inactive
```
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1476797365)
In 5fbf18b89d6768bed1be95dcbc98797df5788dfc "wallet: track mempool conflicts"
I'm a little bit confused by the "erase this wtx entry" part of the comment. AFAICT, no entries are being erased, Perhaps this should say:
```suggestion
// If this wtx has no other mempool conflicts, mark it as inactive
```
✅ nsvrn closed an issue: "IBD stalling issue in v26.0 and assumevalid=0"
(https://github.com/bitcoin/bitcoin/issues/29374)
(https://github.com/bitcoin/bitcoin/issues/29374)
💬 nsvrn commented on issue "IBD stalling issue in v26.0 and assumevalid=0":
(https://github.com/bitcoin/bitcoin/issues/29374#issuecomment-1924808938)
Closed this issue for now, just realized that it seems to be an issue with internet connection going down on the machine. It could be unrelated to Bitcoin software so doesn't make sense to investigate further on this one.
(https://github.com/bitcoin/bitcoin/issues/29374#issuecomment-1924808938)
Closed this issue for now, just realized that it seems to be an issue with internet connection going down on the machine. It could be unrelated to Bitcoin software so doesn't make sense to investigate further on this one.
💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1924814451)
> It seems to me like a more straightforward way to implement this feature would be to add a `bool mempool_conflicts` field to `TxStateInactive` rather than adding a new `TxStateMempoolConflicted` state. The description of `TxStateInactive` says "May conflict with the mempool, or with an unknown block, or be abandoned, never broadcast, or rejected from the mempool for another reason." So the inactive state was meant to handle transactions that could have conflicts or be inactive for a variety of
...
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1924814451)
> It seems to me like a more straightforward way to implement this feature would be to add a `bool mempool_conflicts` field to `TxStateInactive` rather than adding a new `TxStateMempoolConflicted` state. The description of `TxStateInactive` says "May conflict with the mempool, or with an unknown block, or be abandoned, never broadcast, or rejected from the mempool for another reason." So the inactive state was meant to handle transactions that could have conflicts or be inactive for a variety of
...
💬 chrisguida commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1924833293)
@petertodd You appear not to have read my message at all. I will repeat myself inline where necessary.
> You're basically making an argument to altruism.
No, I'm not. I'm appealing to long-term self-interest for all involved. Altruism involves accepting harm to oneself so that others may benefit. Following Core's default mempool policy involves miners sacrificing _short-term profits_ in order to boost _long-term profits_. Please note that, on the contrary, what _you_ are suggesting involve
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1924833293)
@petertodd You appear not to have read my message at all. I will repeat myself inline where necessary.
> You're basically making an argument to altruism.
No, I'm not. I'm appealing to long-term self-interest for all involved. Altruism involves accepting harm to oneself so that others may benefit. Following Core's default mempool policy involves miners sacrificing _short-term profits_ in order to boost _long-term profits_. Please note that, on the contrary, what _you_ are suggesting involve
...
💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1924893374)
> The default mempool policy serves a similar purpose. Can miners circumvent these limits? Absolutely, but there are harsh consequences, and ultimately miners' interests are much better served by simply playing by the rules.
What are the harsh consequences of changing mempool policies in a permissionless network?
> "Convincing more hash power to set -permitbaremultisig=0" is precisely what I'm doing right now, by lobbying for this PR to be merged into bitcoin core. Sitting on our hands and
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1924893374)
> The default mempool policy serves a similar purpose. Can miners circumvent these limits? Absolutely, but there are harsh consequences, and ultimately miners' interests are much better served by simply playing by the rules.
What are the harsh consequences of changing mempool policies in a permissionless network?
> "Convincing more hash power to set -permitbaremultisig=0" is precisely what I'm doing right now, by lobbying for this PR to be merged into bitcoin core. Sitting on our hands and
...
💬 furszy commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#discussion_r1476878728)
in 78ba0e6748d:
I know that this shouldn't fail, but it would be good to log the error if it happens and inform the user in the error message that they need to reload the wallet manually.
(same for the others)
(https://github.com/bitcoin/bitcoin/pull/28868#discussion_r1476878728)
in 78ba0e6748d:
I know that this shouldn't fail, but it would be good to log the error if it happens and inform the user in the error message that they need to reload the wallet manually.
(same for the others)