✅ achow101 closed an issue: "ci: failure in `rpc_scanblocks.py`"
(https://github.com/bitcoin/bitcoin/issues/29831)
(https://github.com/bitcoin/bitcoin/issues/29831)
🚀 achow101 merged a pull request: "index: race fix, lock cs_main while 'm_synced' is subject to change"
(https://github.com/bitcoin/bitcoin/pull/29867)
(https://github.com/bitcoin/bitcoin/pull/29867)
💬 achow101 commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#issuecomment-2077879385)
ACK 7c0c599f1e6d412b3cb92cd67aa46644a8286cb6
(https://github.com/bitcoin/bitcoin/pull/29335#issuecomment-2077879385)
ACK 7c0c599f1e6d412b3cb92cd67aa46644a8286cb6
💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1579936778)
Nice catch!
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1579936778)
Nice catch!
💬 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
...