💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2077888934)
Rebased on top of https://github.com/bitcoin/bitcoin/pull/29736. Now this is using `wait_for_getheaders` to reduce the boilerplate of having to manually pop the results from `last_message`.
cc/ @stratospher
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2077888934)
Rebased on top of https://github.com/bitcoin/bitcoin/pull/29736. Now this is using `wait_for_getheaders` to reduce the boilerplate of having to manually pop the results from `last_message`.
cc/ @stratospher
💬 achow101 commented on pull request "Wallet: (Refactor) GetBalance to calculate used balance":
(https://github.com/bitcoin/bitcoin/pull/29062#discussion_r1579949478)
It would be a bit easier to understand if this subtraction was done with each addition during the loop, rather than all at the end. Also a comment for why this is being done would be useful, as it was not immediately obvious to me at first that the purpose of this is to remove the overlap.
(https://github.com/bitcoin/bitcoin/pull/29062#discussion_r1579949478)
It would be a bit easier to understand if this subtraction was done with each addition during the loop, rather than all at the end. Also a comment for why this is being done would be useful, as it was not immediately obvious to me at first that the purpose of this is to remove the overlap.
💬 achow101 commented on pull request "Wallet: (Refactor) GetBalance to calculate used balance":
(https://github.com/bitcoin/bitcoin/pull/29062#discussion_r1579945607)
This needs to be in the above `if` so that the min_depth check is still being done.
(https://github.com/bitcoin/bitcoin/pull/29062#discussion_r1579945607)
This needs to be in the above `if` so that the min_depth check is still being done.
💬 sr-gi commented on pull request "test: Extends wait_for_getheaders so a specific block hash can be checked":
(https://github.com/bitcoin/bitcoin/pull/29736#issuecomment-2077906602)
> I wonder if it may be worth splitting the `wait_for_getheaders` function so the internal function can also be used externally, so we can use that to check for whether a certain message exists without waiting for them (e.g. checking for the negative case) in a similar fashion as done in [bc844fd](https://github.com/bitcoin/bitcoin/commit/bc844fd8a7d130bfb7cf598f5b1acd87acd70e58)
>
> This way we would get rid of a bunch of manual checks like:
>
> ```python
> with p2p_lock:
> assert "
...
(https://github.com/bitcoin/bitcoin/pull/29736#issuecomment-2077906602)
> I wonder if it may be worth splitting the `wait_for_getheaders` function so the internal function can also be used externally, so we can use that to check for whether a certain message exists without waiting for them (e.g. checking for the negative case) in a similar fashion as done in [bc844fd](https://github.com/bitcoin/bitcoin/commit/bc844fd8a7d130bfb7cf598f5b1acd87acd70e58)
>
> This way we would get rid of a bunch of manual checks like:
>
> ```python
> with p2p_lock:
> assert "
...
💬 sr-gi commented on pull request "test: Makes `wait_for_getdata` delete data on checks, plus allows to check the getdata message type":
(https://github.com/bitcoin/bitcoin/pull/29748#issuecomment-2077916121)
Rebased to fixed merge conflicts after #29736
(https://github.com/bitcoin/bitcoin/pull/29748#issuecomment-2077916121)
Rebased to fixed merge conflicts after #29736
📝 hebasto opened a pull request: "depends: Do not consider `CC` environment variable for detecting system"
(https://github.com/bitcoin/bitcoin/pull/29963)
On the master branch @ 3c88eac28e8984893746caebb313dc3b2fca90db, consider the following commands in the `depends` subdirectory:
```sh
$ make print-build HOST=i686-pc-linux-gnu CC="clang -m32"
build=x86_64-pc-linux-gnu
$ make print-host HOST=i686-pc-linux-gnu CC="clang -m32"
host=i686-pc-linux-gnu
```
The printed variable values are expected.
However, switching the `CC` variable context from Makefile to the shell environment breaks expectations:
```sh
$ CC="clang -m32" make print-buil
...
(https://github.com/bitcoin/bitcoin/pull/29963)
On the master branch @ 3c88eac28e8984893746caebb313dc3b2fca90db, consider the following commands in the `depends` subdirectory:
```sh
$ make print-build HOST=i686-pc-linux-gnu CC="clang -m32"
build=x86_64-pc-linux-gnu
$ make print-host HOST=i686-pc-linux-gnu CC="clang -m32"
host=i686-pc-linux-gnu
```
The printed variable values are expected.
However, switching the `CC` variable context from Makefile to the shell environment breaks expectations:
```sh
$ CC="clang -m32" make print-buil
...
💬 ryanofsky commented on pull request "index: race fix, lock cs_main while 'm_synced' is subject to change":
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2077987295)
Note just to clarify issue history: The race condition fixed here was introduced by 0faafb57f8298547949cbc0044ee9e925ed887ba from #28955, which stopped locking `cs_main` while setting `m_synced = true`. It could cause indexes to incorrectly discard block-connected notifications.
This race condition is different from the other race condition recently fixed in #29776, which is a much older bug going back to 4368384f1d267b011e03a805f934f5c47e2ca1b2 from [#14121](https://github.com/bitcoin/bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2077987295)
Note just to clarify issue history: The race condition fixed here was introduced by 0faafb57f8298547949cbc0044ee9e925ed887ba from #28955, which stopped locking `cs_main` while setting `m_synced = true`. It could cause indexes to incorrectly discard block-connected notifications.
This race condition is different from the other race condition recently fixed in #29776, which is a much older bug going back to 4368384f1d267b011e03a805f934f5c47e2ca1b2 from [#14121](https://github.com/bitcoin/bitcoi
...
💬 theuni commented on pull request "depends: Do not consider `CC` environment variable for detecting system":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2078014617)
Where/why is this an issue?
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2078014617)
Where/why is this an issue?
💬 hebasto commented on pull request "depends: Do not consider `CC` environment variable for detecting system":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2078027076)
> Where/why is this an issue?
I faced this issue during my work on CMake branch when `CC` environment variable was defined by the CI -- https://github.com/hebasto/bitcoin/actions/runs/8822538616/job/24220973382.
The log shows no cross-compiling, but it is cross-compiling to `i686-pc-linux-gnu`.
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2078027076)
> Where/why is this an issue?
I faced this issue during my work on CMake branch when `CC` environment variable was defined by the CI -- https://github.com/hebasto/bitcoin/actions/runs/8822538616/job/24220973382.
The log shows no cross-compiling, but it is cross-compiling to `i686-pc-linux-gnu`.
💬 achow101 commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#issuecomment-2078063941)
ACK fd81a37239541d0d508402cd4eeb28f38128c1f2
(https://github.com/bitcoin/bitcoin/pull/28834#issuecomment-2078063941)
ACK fd81a37239541d0d508402cd4eeb28f38128c1f2
🚀 achow101 merged a pull request: "net: attempts to connect to all resolved addresses when connecting to a node"
(https://github.com/bitcoin/bitcoin/pull/28834)
(https://github.com/bitcoin/bitcoin/pull/28834)
🤔 stickies-v reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2023406729)
Approach ACK, code LGTM 66022315641934bda301d61f009dae9cb3b45da0 with some non-blocking suggestions.
> We could add back a safer, more limited form of operator= in the future if not having it causes friction.
I made my previous comment because I thought there are some instances of somewhat awkward `Update` usage in this PR. The majority of `util::Result` usage is/should probably not require `Update()` at all, because we're just returning either a success value or an error string. But, in t
...
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2023406729)
Approach ACK, code LGTM 66022315641934bda301d61f009dae9cb3b45da0 with some non-blocking suggestions.
> We could add back a safer, more limited form of operator= in the future if not having it causes friction.
I made my previous comment because I thought there are some instances of somewhat awkward `Update` usage in this PR. The majority of `util::Result` usage is/should probably not require `Update()` at all, because we're just returning either a success value or an error string. But, in t
...
💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580022711)
I think using `Update` here is confusing and makes the code less clear.
<details>
<summary>git diff on 6602231564</summary>
```diff
diff --git a/src/init.cpp b/src/init.cpp
index 8cb593754c..678bb2a39c 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -991,10 +991,8 @@ bool AppInitParameterInteraction(const ArgsManager& args)
InitWarning(strprintf(_("Reducing -maxconnections from %d to %d, because of system limitations."), nUserMaxConnections, nMaxConnections));
// *
...
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580022711)
I think using `Update` here is confusing and makes the code less clear.
<details>
<summary>git diff on 6602231564</summary>
```diff
diff --git a/src/init.cpp b/src/init.cpp
index 8cb593754c..678bb2a39c 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -991,10 +991,8 @@ bool AppInitParameterInteraction(const ArgsManager& args)
InitWarning(strprintf(_("Reducing -maxconnections from %d to %d, because of system limitations."), nUserMaxConnections, nMaxConnections));
// *
...
💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580037125)
Alternative approach without `Update`:
<details>
<summary>git diff on 6602231564</summary>
```diff
diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp
index d0b06b726e..d9b4cdfdf2 100644
--- a/src/wallet/test/fuzz/notifications.cpp
+++ b/src/wallet/test/fuzz/notifications.cpp
@@ -106,13 +106,12 @@ struct FuzzedWallet {
CTxDestination GetDestination(FuzzedDataProvider& fuzzed_data_provider)
{
auto type{fuzzed_data_provider
...
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580037125)
Alternative approach without `Update`:
<details>
<summary>git diff on 6602231564</summary>
```diff
diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp
index d0b06b726e..d9b4cdfdf2 100644
--- a/src/wallet/test/fuzz/notifications.cpp
+++ b/src/wallet/test/fuzz/notifications.cpp
@@ -106,13 +106,12 @@ struct FuzzedWallet {
CTxDestination GetDestination(FuzzedDataProvider& fuzzed_data_provider)
{
auto type{fuzzed_data_provider
...
💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580027359)
I don't think `Update` is appropriate here, would e.g. use:
<details>
<summary>git diff on 6602231564</summary>
```diff
diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp
index 2858494625..41ffb76762 100644
--- a/src/test/mempool_tests.cpp
+++ b/src/test/mempool_tests.cpp
@@ -202,9 +202,11 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest)
tx7.vout[1].scriptPubKey = CScript() << OP_11 << OP_EQUAL;
tx7.vout[1].nValue = 1 * COIN;
- auto ancestors_calculat
...
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580027359)
I don't think `Update` is appropriate here, would e.g. use:
<details>
<summary>git diff on 6602231564</summary>
```diff
diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp
index 2858494625..41ffb76762 100644
--- a/src/test/mempool_tests.cpp
+++ b/src/test/mempool_tests.cpp
@@ -202,9 +202,11 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest)
tx7.vout[1].scriptPubKey = CScript() << OP_11 << OP_EQUAL;
tx7.vout[1].nValue = 1 * COIN;
- auto ancestors_calculat
...
💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580043133)
And also here, I think `Update` is confusing and can e.g. be simplified with:
<details>
<summary>git diff on 6602231564</summary>
```diff
diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp
index 640e63903a..963c0f838b 100644
--- a/src/wallet/test/spend_tests.cpp
+++ b/src/wallet/test/spend_tests.cpp
@@ -97,13 +97,11 @@ BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup)
// so that the recipient's amount is no longer equal
...
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580043133)
And also here, I think `Update` is confusing and can e.g. be simplified with:
<details>
<summary>git diff on 6602231564</summary>
```diff
diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp
index 640e63903a..963c0f838b 100644
--- a/src/wallet/test/spend_tests.cpp
+++ b/src/wallet/test/spend_tests.cpp
@@ -97,13 +97,11 @@ BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup)
// so that the recipient's amount is no longer equal
...
💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580051611)
> I haven't seen uses for a Replace method.
I agree. The use cases I was thinking of should just be addressed with list initialization, so I think not implementing the method until we actually need it is the way to go, thanks.
> Second, I think the name Merge would be misleading ... only different fields are combined
That's a good point, I agree `Update` > `Merge`
> it's less obvious what a.Combine(b) would be supposed to do
I don't think I agree, but I do agree there's no clear w
...
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580051611)
> I haven't seen uses for a Replace method.
I agree. The use cases I was thinking of should just be addressed with list initialization, so I think not implementing the method until we actually need it is the way to go, thanks.
> Second, I think the name Merge would be misleading ... only different fields are combined
That's a good point, I agree `Update` > `Merge`
> it's less obvious what a.Combine(b) would be supposed to do
I don't think I agree, but I do agree there's no clear w
...
💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#issuecomment-2078096227)
Moving this to draft given it is dependant on https://github.com/bitcoin/bitcoin/pull/28016
(https://github.com/bitcoin/bitcoin/pull/29605#issuecomment-2078096227)
Moving this to draft given it is dependant on https://github.com/bitcoin/bitcoin/pull/28016
📝 sr-gi converted_to_draft a pull request: "net: Favor peers from addrman over fetching seednodes"
(https://github.com/bitcoin/bitcoin/pull/29605)
This is a follow-up of #28016 motivated by https://github.com/bitcoin/bitcoin/pull/28016#pullrequestreview-1913140932 and https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-1984448937.
The current behavior of seednode fetching is pretty eager: we do it as the first step under `ThreadOpenNetworkConnections` even if some peers may be queryable from our addrman. This poses two potential issues:
- First, if permanently set (e.g. running with seednode in a config file) we'd be signaling
...
(https://github.com/bitcoin/bitcoin/pull/29605)
This is a follow-up of #28016 motivated by https://github.com/bitcoin/bitcoin/pull/28016#pullrequestreview-1913140932 and https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-1984448937.
The current behavior of seednode fetching is pretty eager: we do it as the first step under `ThreadOpenNetworkConnections` even if some peers may be queryable from our addrman. This poses two potential issues:
- First, if permanently set (e.g. running with seednode in a config file) we'd be signaling
...
💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2078100935)
Thanks for taking another look at @ryanofsky!
Updated 30a1024b63105a97d04149e83ae2d8cf0830cf69 -> d447bdcfb0e38353940e4a7fc89d09482d8d39c3 ([mempoolArgs_5](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_5) -> [mempoolArgs_6](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/mempoolArgs_5..mempoolArgs_6))
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2022700917
...
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2078100935)
Thanks for taking another look at @ryanofsky!
Updated 30a1024b63105a97d04149e83ae2d8cf0830cf69 -> d447bdcfb0e38353940e4a7fc89d09482d8d39c3 ([mempoolArgs_5](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_5) -> [mempoolArgs_6](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/mempoolArgs_5..mempoolArgs_6))
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2022700917
...