💬 mzumsande commented on pull request "wallet: Fix error messages telling user to specify wallet":
(https://github.com/bitcoin/bitcoin/pull/30912#issuecomment-2354152837)
How can it be a duplicate if it still says "filename" everywhere after the first commit of #26990? I mean I'm happy to close this trivial PR if #26990 wants to rename that too (if it's correct to do so) but how can they be duplicates if they do different things?!
(https://github.com/bitcoin/bitcoin/pull/30912#issuecomment-2354152837)
How can it be a duplicate if it still says "filename" everywhere after the first commit of #26990? I mean I'm happy to close this trivial PR if #26990 wants to rename that too (if it's correct to do so) but how can they be duplicates if they do different things?!
💬 jonatack commented on pull request "wallet: Fix error messages telling user to specify wallet":
(https://github.com/bitcoin/bitcoin/pull/30912#issuecomment-2354178930)
As they both touch the same lines, I thought you may not have been aware of that pull and that the natural course of action would be to leave feedback there rather than both pulls proposing to improve the same message. If you disagree with that I apologize.
(https://github.com/bitcoin/bitcoin/pull/30912#issuecomment-2354178930)
As they both touch the same lines, I thought you may not have been aware of that pull and that the natural course of action would be to leave feedback there rather than both pulls proposing to improve the same message. If you disagree with that I apologize.
💬 mzumsande commented on pull request "wallet: Fix error messages telling user to specify wallet":
(https://github.com/bitcoin/bitcoin/pull/30912#issuecomment-2354207416)
> If you disagree with that I apologize.
I wasn't aware of the PR and actually agree with that, I just thought (already from the first comment) you wanted to say that the PRs are duplicates, as in addressing the same issue, which they are not.
(https://github.com/bitcoin/bitcoin/pull/30912#issuecomment-2354207416)
> If you disagree with that I apologize.
I wasn't aware of the PR and actually agree with that, I just thought (already from the first comment) you wanted to say that the PRs are duplicates, as in addressing the same issue, which they are not.
✅ mzumsande closed a pull request: "wallet: Fix error messages telling user to specify wallet"
(https://github.com/bitcoin/bitcoin/pull/30912)
(https://github.com/bitcoin/bitcoin/pull/30912)
💬 mzumsande 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_r1762081524)
I found "filename" confusing, see #30912 - if there isn't a deeper reason behind it that I'm unaware of, it could be renamed to `walletname`.
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1762081524)
I found "filename" confusing, see #30912 - if there isn't a deeper reason behind it that I'm unaware of, it could be renamed to `walletname`.
💬 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_r1762106926)
Good point. I was curious, as I've moved this error message around a few times for different refactorings; it looks like it originated back in https://github.com/bitcoin/bitcoin/pull/10931.
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1762106926)
Good point. I was curious, as I've moved this error message around a few times for different refactorings; it looks like it originated back in https://github.com/bitcoin/bitcoin/pull/10931.
💬 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_r1762109275)
@pablomartin4btc maybe a test could be added to `interface_bitcoin_cli.py` that passing a filename raises (error code -18 "Requested wallet does not exist or is not loaded").
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1762109275)
@pablomartin4btc maybe a test could be added to `interface_bitcoin_cli.py` that passing a filename raises (error code -18 "Requested wallet does not exist or is not loaded").
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1762117682)
good catch!! that was unintentional, reverted now
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1762117682)
good catch!! that was unintentional, reverted now
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1762117778)
done
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1762117778)
done
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1762118393)
done
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1762118393)
done
✅ glozow closed a pull request: "TxDownloadManager refactor followups"
(https://github.com/bitcoin/bitcoin/pull/30820)
(https://github.com/bitcoin/bitcoin/pull/30820)
💬 glozow commented on pull request "TxDownloadManager refactor followups":
(https://github.com/bitcoin/bitcoin/pull/30820#issuecomment-2354268095)
closing, squashed into #30110. will reopen if followups need to happen again.
(https://github.com/bitcoin/bitcoin/pull/30820#issuecomment-2354268095)
closing, squashed into #30110. will reopen if followups need to happen again.
💬 furszy 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_r1762123098)
I think it would be beneficial to explain the alternatives. The `-rpcwallet` parameter depends on how the wallet is loaded. It can either be the wallet directory name located inside the `-walletdir` (the default value is `datadir/net/wallets/wallet_name`, in which case only `wallet_name` is used in this option), or the absolute path to the wallet directory, which is the complete path `datadir/net/wallets/wallet_name`.
You can note the difference by loading an existing wallet in two different
...
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1762123098)
I think it would be beneficial to explain the alternatives. The `-rpcwallet` parameter depends on how the wallet is loaded. It can either be the wallet directory name located inside the `-walletdir` (the default value is `datadir/net/wallets/wallet_name`, in which case only `wallet_name` is used in this option), or the absolute path to the wallet directory, which is the complete path `datadir/net/wallets/wallet_name`.
You can note the difference by loading an existing wallet in two different
...
👋 theuni's pull request is ready for review: "build: speed up by flattening the dependency graph"
(https://github.com/bitcoin/bitcoin/pull/30911)
(https://github.com/bitcoin/bitcoin/pull/30911)
💬 mzumsande 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_r1762156524)
... and I think in both of these cases `walletname` would be correct and `filename` incorrect, it would just be a different wallet name depending on how it's loaded.
It could actually also be a filename: Put a legacy wallet `oldwallet.dat` into the datadir, run with `-deprecatedrpc=create_bdb` and `loadwallet "oldwallet.dat"` will load it successfully, in this special case the filename is the walletname. But even here, where `filename` would make sense, `walletname` is correct too, so it just
...
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1762156524)
... and I think in both of these cases `walletname` would be correct and `filename` incorrect, it would just be a different wallet name depending on how it's loaded.
It could actually also be a filename: Put a legacy wallet `oldwallet.dat` into the datadir, run with `-deprecatedrpc=create_bdb` and `loadwallet "oldwallet.dat"` will load it successfully, in this special case the filename is the walletname. But even here, where `filename` would make sense, `walletname` is correct too, so it just
...
💬 davidgumberg commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#issuecomment-2354365190)
Concept ACK
Nit: could also fix
```diff
--- a/test/util/test_runner.py
+++ b/test/util/test_runner.py
@@ -5,7 +5,7 @@
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Test framework for bitcoin utils.
-Runs automatically during `make check`.
+Runs automatically during `ctest --test-dir build/`.
Can also be run manually."""
```
(https://github.com/bitcoin/bitcoin/pull/30875#issuecomment-2354365190)
Concept ACK
Nit: could also fix
```diff
--- a/test/util/test_runner.py
+++ b/test/util/test_runner.py
@@ -5,7 +5,7 @@
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Test framework for bitcoin utils.
-Runs automatically during `make check`.
+Runs automatically during `ctest --test-dir build/`.
Can also be run manually."""
```
✅ achow101 closed an issue: "28.0rc1 synchronizes much slower on Windows"
(https://github.com/bitcoin/bitcoin/issues/30833)
(https://github.com/bitcoin/bitcoin/issues/30833)
🚀 achow101 merged a pull request: "streams: cache file position within AutoFile"
(https://github.com/bitcoin/bitcoin/pull/30884)
(https://github.com/bitcoin/bitcoin/pull/30884)
👋 achow101's pull request is ready for review: "[28.x] Further backports and rc2"
(https://github.com/bitcoin/bitcoin/pull/30827)
(https://github.com/bitcoin/bitcoin/pull/30827)
💬 achow101 commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2354420266)
Backported in #30827
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2354420266)
Backported in #30827