💬 foolbear commented on issue "Load PSBT error: Unable to decode PSBT":
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-2192158192)
1. run bitcoin core in prune mode
2. create a new blank wallet
3. importprivkey from an old private key
4. migratewallet
5. re-sync blockchain
6. show the balance
7. create a transaction using GUI, click "send" and "create unsigned".
8. then "Load PSBT from clipboard" from menu, error fired.
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-2192158192)
1. run bitcoin core in prune mode
2. create a new blank wallet
3. importprivkey from an old private key
4. migratewallet
5. re-sync blockchain
6. show the balance
7. create a transaction using GUI, click "send" and "create unsigned".
8. then "Load PSBT from clipboard" from menu, error fired.
💬 tdb3 commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1655201862)
Looks like that's the block hash for block 1000 (mainnet). What's the regtest minimum difficulty? The hash that we don't want to be found should be something that definitely isn't possible.
Would also be good to inform the log, for example:
```diff
diff --git a/test/functional/feature_coinstatsindex.py b/test/functional/feature_coinstatsindex.py
index c6edbe7c31..8e37a3c537 100755
--- a/test/functional/feature_coinstatsindex.py
+++ b/test/functional/feature_coinstatsindex.py
@@ -242,6
...
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1655201862)
Looks like that's the block hash for block 1000 (mainnet). What's the regtest minimum difficulty? The hash that we don't want to be found should be something that definitely isn't possible.
Would also be good to inform the log, for example:
```diff
diff --git a/test/functional/feature_coinstatsindex.py b/test/functional/feature_coinstatsindex.py
index c6edbe7c31..8e37a3c537 100755
--- a/test/functional/feature_coinstatsindex.py
+++ b/test/functional/feature_coinstatsindex.py
@@ -242,6
...
🚀 achow101 merged a pull request: "Update libsecp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/30334)
(https://github.com/bitcoin/bitcoin/pull/30334)
💬 paplorinc commented on pull request "WIP: Simplify SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1655203435)
Thanks @sipa, do you agree that
```
static void SipHash_32b(benchmark::Bench& bench)
{
FastRandomContext rng(true);
auto k0{rng.rand64()}, k1{rng.rand64()};
auto x{rng.rand256()};
auto* x_ptr{reinterpret_cast<uint64_t*>(x.data())};
bench.run([&] {
ankerl::nanobench::doNotOptimizeAway(SipHashUint256(k0, k1, x));
++k0; ++k1; ++x_ptr[0]; ++x_ptr[1]; ++x_ptr[2]; ++x_ptr[3];
});
}
```
likely produces more realistic results (see above results)?
Th
...
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1655203435)
Thanks @sipa, do you agree that
```
static void SipHash_32b(benchmark::Bench& bench)
{
FastRandomContext rng(true);
auto k0{rng.rand64()}, k1{rng.rand64()};
auto x{rng.rand256()};
auto* x_ptr{reinterpret_cast<uint64_t*>(x.data())};
bench.run([&] {
ankerl::nanobench::doNotOptimizeAway(SipHashUint256(k0, k1, x));
++k0; ++k1; ++x_ptr[0]; ++x_ptr[1]; ++x_ptr[2]; ++x_ptr[3];
});
}
```
likely produces more realistic results (see above results)?
Th
...
👍 hodlinator approved a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2142445372)
ACK 325152a2fd965dcccf71b0f599dc91cef0441862
`git range-diff a0dffe6~1..a0dffe6 325152a~1..325152a` ✅
<details>
<summary>
Output on Linux:
</summary>
```console
$ bitcoin-cli -regtest loadwallet
error code: -1
error message:
loadwallet "filename" ( load_on_startup )
Loads a wallet from a wallet file or directory.
Note that all wallet command-line options used when starting bitcoind will be
applied to the new wallet.
Arguments:
1. filename (string, required) You
...
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2142445372)
ACK 325152a2fd965dcccf71b0f599dc91cef0441862
`git range-diff a0dffe6~1..a0dffe6 325152a~1..325152a` ✅
<details>
<summary>
Output on Linux:
</summary>
```console
$ bitcoin-cli -regtest loadwallet
error code: -1
error message:
loadwallet "filename" ( load_on_startup )
Loads a wallet from a wallet file or directory.
Note that all wallet command-line options used when starting bitcoind will be
applied to the new wallet.
Arguments:
1. filename (string, required) You
...
💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1655214521)
nit: Might be good to add quotes around the path like the other examples in case there are spaces in usernames or other part of the directory?
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1655214521)
nit: Might be good to add quotes around the path like the other examples in case there are spaces in usernames or other part of the directory?
💬 maflcko commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1655244618)
I don't think the RPC documentation in the right place to inject and leak the name of the user that generated the docs. Among the obvious issues, this also makes them less deterministic.
I am sure a user can figure out what an absolute path means without having to see it in the docs.
If it was important to show this location, it can be returned by a dedicated RPC field?
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1655244618)
I don't think the RPC documentation in the right place to inject and leak the name of the user that generated the docs. Among the obvious issues, this also makes them less deterministic.
I am sure a user can figure out what an absolute path means without having to see it in the docs.
If it was important to show this location, it can be returned by a dedicated RPC field?
💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1655262092)
I wonder if the option should be renamed `-privaterpcbroadcast` for now, and we can add `-privatewalletbroadcast` later.
We also won't be able to prevent users from using their wallet with `sendrawtransaction` anyway. So if the goal here is to prevent accidental privacy leaks from confused users, we might want to consider "hiding" the option or restricting it to test networks until wallet integration is complete.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1655262092)
I wonder if the option should be renamed `-privaterpcbroadcast` for now, and we can add `-privatewalletbroadcast` later.
We also won't be able to prevent users from using their wallet with `sendrawtransaction` anyway. So if the goal here is to prevent accidental privacy leaks from confused users, we might want to consider "hiding" the option or restricting it to test networks until wallet integration is complete.
💬 maflcko commented on pull request "guix: use GCC 13 to builds releases":
(https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1655265995)
from CI:
```
bitcoin-cli.cpp:767:13: error: ‘result’ may be used uninitialized [-Werror=maybe-uninitialized]
(https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1655265995)
from CI:
```
bitcoin-cli.cpp:767:13: error: ‘result’ may be used uninitialized [-Werror=maybe-uninitialized]
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655273411)
Will add, thanks!
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655273411)
Will add, thanks!
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655277307)
i don't disagree with doing this, but `netif.h` is only included in two places: `net.cpp` and `pcp.cpp`, both of which use `netaddress.h` anyway. Not sure it's worth adding a change in `netaddress.h` in this PR for.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655277307)
i don't disagree with doing this, but `netif.h` is only included in two places: `net.cpp` and `pcp.cpp`, both of which use `netaddress.h` anyway. Not sure it's worth adding a change in `netaddress.h` in this PR for.
💬 achow101 commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655286366)
In 31cc8bc305103baf2c9979d86ef1b43dec5628cb "test: add rpccookieperms test"
This is incorrect. It causes the entire test file to be marked as skipped (different exit code, marked differently in the test runner output) even though everything else ran. The correct thing here is to skip this individual case, which can be achieved by simply returning.
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655286366)
In 31cc8bc305103baf2c9979d86ef1b43dec5628cb "test: add rpccookieperms test"
This is incorrect. It causes the entire test file to be marked as skipped (different exit code, marked differently in the test runner output) even though everything else ran. The correct thing here is to skip this individual case, which can be achieved by simply returning.
💬 achow101 commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655279201)
In 31cc8bc305103baf2c9979d86ef1b43dec5628cb "test: add rpccookieperms test"
Since #29034, the preferred convention is to use `platform.system()` rather than `os.name`.
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655279201)
In 31cc8bc305103baf2c9979d86ef1b43dec5628cb "test: add rpccookieperms test"
Since #29034, the preferred convention is to use `platform.system()` rather than `os.name`.
💬 achow101 commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655288219)
In 38af55e285336cdd3f42635938f24622451703b0 "util: add PermsToString and StringToPerms"
nit: This naming is a little bit confusing to me since it suggests that these functions are inverses of each other, but they're not as their string outputs are not compatible.
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655288219)
In 38af55e285336cdd3f42635938f24622451703b0 "util: add PermsToString and StringToPerms"
nit: This naming is a little bit confusing to me since it suggests that these functions are inverses of each other, but they're not as their string outputs are not compatible.
💬 achow101 commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655290944)
In 7ad5349a2ec04399c99c8cbf71c8d3b4627132f8 "init: add option for rpccookie permissions"
This comment is removed, but the umask is still what sets the permissions in the default case.
Since we can set the permissions explicitly in this function, I think it would make sense to always set them with `owner` as default rather than relying on the umask that happens somewhere else.
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1655290944)
In 7ad5349a2ec04399c99c8cbf71c8d3b4627132f8 "init: add option for rpccookie permissions"
This comment is removed, but the umask is still what sets the permissions in the default case.
Since we can set the permissions explicitly in this function, I think it would make sense to always set them with `owner` as default rather than relying on the umask that happens somewhere else.
📝 ryanofsky opened a pull request: "kernel, logging: Pass Logger instances to kernel objects"
(https://github.com/bitcoin/bitcoin/pull/30342)
Pass `Logger` instances to `BlockManager`, `CCoinsViewDB`, `CDBWrapper`, `Chainstate`, `ChainstateManager`, `CoinsViews`, and `CTxMemPool` classes so libbitcoinkernel applications and tests have the option to control where log output goes instead of having all output sent to the global logger.
This PR is an alternative to #30338. It is more limited because it only changes kernel code while leaving other parts of the codebase alone. It also takes the opportunity to migrate legacy LogPrint / Lo
...
(https://github.com/bitcoin/bitcoin/pull/30342)
Pass `Logger` instances to `BlockManager`, `CCoinsViewDB`, `CDBWrapper`, `Chainstate`, `ChainstateManager`, `CoinsViews`, and `CTxMemPool` classes so libbitcoinkernel applications and tests have the option to control where log output goes instead of having all output sent to the global logger.
This PR is an alternative to #30338. It is more limited because it only changes kernel code while leaving other parts of the codebase alone. It also takes the opportunity to migrate legacy LogPrint / Lo
...
💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1655304597)
Hadn't thought of the doc-generation aspect and leaks, good point.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1655304597)
Hadn't thought of the doc-generation aspect and leaks, good point.
📝 ryanofsky opened a pull request: "wallet, logging: Replace WalletLogPrintf() with LogInfo()"
(https://github.com/bitcoin/bitcoin/pull/30343)
Make new logging macros `LogDebug()`, `LogTrace()`, `LogInfo()`, `LogWarning()`, and `LogError()` compatible with wallet code and drop custom WalletLogPrintf() function. The new logging macros introduced in #28318 weren't previously useful in wallet code because wallet code prefixes most log messages with the wallet names to be be able to distinguish log output from multiple wallets. Fix this by introducing a new `WalletLogSource` class inheriting from `BCLog::Source` which can include the walle
...
(https://github.com/bitcoin/bitcoin/pull/30343)
Make new logging macros `LogDebug()`, `LogTrace()`, `LogInfo()`, `LogWarning()`, and `LogError()` compatible with wallet code and drop custom WalletLogPrintf() function. The new logging macros introduced in #28318 weren't previously useful in wallet code because wallet code prefixes most log messages with the wallet names to be be able to distinguish log output from multiple wallets. Fix this by introducing a new `WalletLogSource` class inheriting from `BCLog::Source` which can include the walle
...
💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1655317117)
Just realized that in the console output in my latest test https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2142445372, the RPC `--user` is hardcoded to `myusername`:
```
> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "loadwallet", "params": [/home/hodlinator/.bitcoin/specialWallet/]}'
```
So might as well use the same name if we want to go back to hardcoded platform-specific paths.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1655317117)
Just realized that in the console output in my latest test https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2142445372, the RPC `--user` is hardcoded to `myusername`:
```
> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "loadwallet", "params": [/home/hodlinator/.bitcoin/specialWallet/]}'
```
So might as well use the same name if we want to go back to hardcoded platform-specific paths.
💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2192368771)
With the latest push, this PR is now much smaller, and now mostly consists of documentation and test changes.
I wanted to split this up in order to be able to base #30342 on a smaller changeset. The wallet changes that were previously part of this PR were moved to #30343.
Rebased ce0004e42d37786f98cdf9d55b976b935dcf1ba1 -> 5f64eab013a58967af37a59c863c2ab6d45ba6e8 ([`pr/bclog.16`](https://github.com/ryanofsky/bitcoin/commits/pr/bclog.16) -> [`pr/bclog.17`](https://github.com/ryanofsky/bitco
...
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2192368771)
With the latest push, this PR is now much smaller, and now mostly consists of documentation and test changes.
I wanted to split this up in order to be able to base #30342 on a smaller changeset. The wallet changes that were previously part of this PR were moved to #30343.
Rebased ce0004e42d37786f98cdf9d55b976b935dcf1ba1 -> 5f64eab013a58967af37a59c863c2ab6d45ba6e8 ([`pr/bclog.16`](https://github.com/ryanofsky/bitcoin/commits/pr/bclog.16) -> [`pr/bclog.17`](https://github.com/ryanofsky/bitco
...