💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1655069069)
sounds good resolved in [6809ffd](https://github.com/bitcoin/bitcoin/pull/30340/commits/6809ffd8a6c589c515af48da2dd8367e4c6c2c26)
(https://github.com/bitcoin/bitcoin/pull/30340#discussion_r1655069069)
sounds good resolved in [6809ffd](https://github.com/bitcoin/bitcoin/pull/30340/commits/6809ffd8a6c589c515af48da2dd8367e4c6c2c26)
💬 theuni commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655081131)
Missing:
```c++
#include <cstdint>
#include <chrono>
#include <array>
```
(Noticed while trying to forward-declare `netaddress.h`, which didn't work because of the `CService`s stored in `MappingResult`)
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655081131)
Missing:
```c++
#include <cstdint>
#include <chrono>
#include <array>
```
(Noticed while trying to forward-declare `netaddress.h`, which didn't work because of the `CService`s stored in `MappingResult`)
💬 theuni commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655098222)
This header can be made dependency-free (while making `netaddress.h` a little more forward-declare-friendly) with:
```diff
diff --git a/src/netaddress.h b/src/netaddress.h
index 52fecada1c9..75d172ab334 100644
--- a/src/netaddress.h
+++ b/src/netaddress.h
@@ -31,3 +31,3 @@
*/
-enum Network {
+enum Network : int {
/// Addresses from these networks are not publicly routable on the global Internet.
diff --git a/src/util/netif.cpp b/src/util/netif.cpp
index ad3f93b379a..c4d4c3a31c
...
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655098222)
This header can be made dependency-free (while making `netaddress.h` a little more forward-declare-friendly) with:
```diff
diff --git a/src/netaddress.h b/src/netaddress.h
index 52fecada1c9..75d172ab334 100644
--- a/src/netaddress.h
+++ b/src/netaddress.h
@@ -31,3 +31,3 @@
*/
-enum Network {
+enum Network : int {
/// Addresses from these networks are not publicly routable on the global Internet.
diff --git a/src/util/netif.cpp b/src/util/netif.cpp
index ad3f93b379a..c4d4c3a31c
...
💬 pinheadmz commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1655133216)
I'm actually in favor of adding `tor_port()` to `util.py` and binding every bitcoind in the functional tests to a unique port by default. It is consistent with how we set the other two ports and feels like an oversight not to assign tor port the same way, as long as it doesn't interfere with anything else.
I know what you mean that its not needed but it also default behavior to bind a tor port so I feel like we should do that in the functional tests to prevent some future regression.
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1655133216)
I'm actually in favor of adding `tor_port()` to `util.py` and binding every bitcoind in the functional tests to a unique port by default. It is consistent with how we set the other two ports and feels like an oversight not to assign tor port the same way, as long as it doesn't interfere with anything else.
I know what you mean that its not needed but it also default behavior to bind a tor port so I feel like we should do that in the functional tests to prevent some future regression.
💬 achow101 commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#issuecomment-2192067333)
ACK 72b226882fe2348a9a66aee1d8d21b4e2d275e68
(https://github.com/bitcoin/bitcoin/pull/30309#issuecomment-2192067333)
ACK 72b226882fe2348a9a66aee1d8d21b4e2d275e68
💬 paplorinc commented on pull request "WIP: Simplify SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1655154623)
@maflcko, so my point was that it seems to me that changing the benchmark slightly changes it's performance considerably:
```C++
static void SipHash_32b(benchmark::Bench& bench)
{
uint256 x;
uint64_t k1 = 0;
bench.run([&] {
*((uint64_t*)x.begin()) = SipHashUint256(0, ++k1, x);
});
}
```
works with inputs such as:
<img src="https://github.com/bitcoin/bitcoin/assets/1841944/66d3022e-fa15-4950-bf12-c7db9896d766">
and the benchmark results in:
> make -j10 && .
...
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1655154623)
@maflcko, so my point was that it seems to me that changing the benchmark slightly changes it's performance considerably:
```C++
static void SipHash_32b(benchmark::Bench& bench)
{
uint256 x;
uint64_t k1 = 0;
bench.run([&] {
*((uint64_t*)x.begin()) = SipHashUint256(0, ++k1, x);
});
}
```
works with inputs such as:
<img src="https://github.com/bitcoin/bitcoin/assets/1841944/66d3022e-fa15-4950-bf12-c7db9896d766">
and the benchmark results in:
> make -j10 && .
...
💬 maflcko commented on pull request "WIP: Simplify SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1655160631)
Yes, this is expected. If you change the code that is benchmarked, the compiler will compile it to something else, which will perform differently, because it calculates something else.
(https://github.com/bitcoin/bitcoin/pull/30317#discussion_r1655160631)
Yes, this is expected. If you change the code that is benchmarked, the compiler will compile it to something else, which will perform differently, because it calculates something else.
🚀 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]