🚀 achow101 merged a pull request: "wallet: notify when preset + automatic inputs exceed max weight"
(https://github.com/bitcoin/bitcoin/pull/30309)
(https://github.com/bitcoin/bitcoin/pull/30309)
💬 achow101 commented on pull request "Update libsecp256k1 subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/30334#issuecomment-2192114591)
ACK cc58e958f341d2759fbabe5c9d8cc557e17d587f
Verified that there is no diff between the subtree and secp master.
(https://github.com/bitcoin/bitcoin/pull/30334#issuecomment-2192114591)
ACK cc58e958f341d2759fbabe5c9d8cc557e17d587f
Verified that there is no diff between the subtree and secp master.
💬 paplorinc commented on pull request "WIP: Simplify SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1655183244)
Can you please be more specific that this?
My intention is to measure `SipHash` more realistically, since it didn't behave as I expected before.
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1655183244)
Can you please be more specific that this?
My intention is to measure `SipHash` more realistically, since it didn't behave as I expected before.
💬 sipa commented on pull request "WIP: Simplify SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1655188978)
For very simple functions, variations in performance that are just due to e.g. how code is aligned in memory cannot be avoided.
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1655188978)
For very simple functions, variations in performance that are just due to e.g. how code is aligned in memory cannot be avoided.
💬 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
...