🤔 ismaelsadeeq reviewed a pull request: "Cluster linearization: separate tests from tests-of-tests"
(https://github.com/bitcoin/bitcoin/pull/30605#pullrequestreview-2347653035)
I reviewed the latest changes in the last push
Code review 8eb70e3ca467ac66bc1f43e3e0b3338d63806ee5
Changes since my last review were
1. Addressed my previous review comments.
2. An optimization was introduced in the `clusterlin_simple_linearize` fuzz test that skips to the last non-topological permutation.
For example, given transactions `[A, B, C, D, E]`.
if A-B-C are a valid topological ancestor set, and the first permutation results in [A, E, B, C, D], which is not topologica
...
(https://github.com/bitcoin/bitcoin/pull/30605#pullrequestreview-2347653035)
I reviewed the latest changes in the last push
Code review 8eb70e3ca467ac66bc1f43e3e0b3338d63806ee5
Changes since my last review were
1. Addressed my previous review comments.
2. An optimization was introduced in the `clusterlin_simple_linearize` fuzz test that skips to the last non-topological permutation.
For example, given transactions `[A, B, C, D, E]`.
if A-B-C are a valid topological ancestor set, and the first permutation results in [A, E, B, C, D], which is not topologica
...
💬 ismaelsadeeq commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787549994)
We can cache the subset and then reuse it here and when finding topological subset to be deleted.
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787549994)
We can cache the subset and then reuse it here and when finding topological subset to be deleted.
💬 ismaelsadeeq commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787476002)
Is this not $O(N * 2{^N})$
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787476002)
Is this not $O(N * 2{^N})$
💬 ismaelsadeeq commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787508540)
I might be missing something here, so I would appreciate some clarification.
1. Why don't we directly remove the candidate set transactions found by the search?
2. It seems like there could be multiple valid topological sets at a given time. How does it know which set was actually removed?
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787508540)
I might be missing something here, so I would appreciate some clarification.
1. Why don't we directly remove the candidate set transactions found by the search?
2. It seems like there could be multiple valid topological sets at a given time. How does it know which set was actually removed?
💬 ismaelsadeeq commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787551272)
Same comment applies here as well.
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787551272)
Same comment applies here as well.
💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1787577470)
Should be unambigious now, i've renamed it and the methods to set/clear the mock time are included on the clock itself.
```c++
/**
* Version of SteadyClock that is mockable in the context of tests (set the
* current value with SetMockSteadyTime), otherwise the system steady clock.
*/
struct MockableSteadyClock : public std::chrono::steady_clock {
using time_point = std::chrono::time_point<MockableSteadyClock>;
/** Return current system time or mocked time, if set */
stati
...
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1787577470)
Should be unambigious now, i've renamed it and the methods to set/clear the mock time are included on the clock itself.
```c++
/**
* Version of SteadyClock that is mockable in the context of tests (set the
* current value with SetMockSteadyTime), otherwise the system steady clock.
*/
struct MockableSteadyClock : public std::chrono::steady_clock {
using time_point = std::chrono::time_point<MockableSteadyClock>;
/** Return current system time or mocked time, if set */
stati
...
💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787606089)
Yeah, it's $\mathcal{O}(2^N)$ *iterations*, but overall runtime is indeed $\mathcal{O}(N \cdot 2^N)$ (or, arguably, $\mathcal{O}(N^2 \cdot 2^N)$ even if you take into account that for larger $N$, the bitsets involved need to grow too.
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787606089)
Yeah, it's $\mathcal{O}(2^N)$ *iterations*, but overall runtime is indeed $\mathcal{O}(N \cdot 2^N)$ (or, arguably, $\mathcal{O}(N^2 \cdot 2^N)$ even if you take into account that for larger $N$, the bitsets involved need to grow too.
💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787610478)
The goal here is simply continuing the test, so this isn't just exercising the first candidate found for a linearization, but can hit arbitrary remainders in later iterations too. To that end, we read a topological subset from the fuzzer, which can be anything, but since `ReadTopologicalSet` may return an empty set, we need to specially deal with that case.
One possibility would be adding to `ReadTopologicalSet` a way to force it to return a non-empty set. Instead, the code here uses the foun
...
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787610478)
The goal here is simply continuing the test, so this isn't just exercising the first candidate found for a linearization, but can hit arbitrary remainders in later iterations too. To that end, we read a topological subset from the fuzzer, which can be anything, but since `ReadTopologicalSet` may return an empty set, we need to specially deal with that case.
One possibility would be adding to `ReadTopologicalSet` a way to force it to return a non-empty set. Instead, the code here uses the foun
...
📝 hebasto opened a pull request: "qt6: Handle different signatures of `QANEF::nativeEventFilter`"
(https://github.com/bitcoin-core/gui/pull/840)
Split from https://github.com/bitcoin/bitcoin/pull/30997.
This PR ensures compatibility across all supported Qt versions.
For more details, please refer to https://github.com/qt/qtbase/commit/3b38c73c7ffa71c00c172cf0e05742835a304300.
No behaviour change.
(https://github.com/bitcoin-core/gui/pull/840)
Split from https://github.com/bitcoin/bitcoin/pull/30997.
This PR ensures compatibility across all supported Qt versions.
For more details, please refer to https://github.com/qt/qtbase/commit/3b38c73c7ffa71c00c172cf0e05742835a304300.
No behaviour change.
💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787611090)
Sure we could, but I don't think there is reason to assume that's necessarily the best choice.
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787611090)
Sure we could, but I don't think there is reason to assume that's necessarily the best choice.
💬 fanquake commented on pull request "qt6: Handle different signatures of `QANEF::nativeEventFilter`":
(https://github.com/bitcoin-core/gui/pull/840#issuecomment-2393573834)
If Qt 6.7.0 is going to be required for Windows after the switchover; why change this code now just to immediately drop it after migrating to Qt 6?
(https://github.com/bitcoin-core/gui/pull/840#issuecomment-2393573834)
If Qt 6.7.0 is going to be required for Windows after the switchover; why change this code now just to immediately drop it after migrating to Qt 6?
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1787644002)
Here is the change:
<details>
<summary>[patch] only remove if accepted in mempool</summary>
```diff
--- i/src/net_processing.cpp
+++ w/src/net_processing.cpp
@@ -4803,27 +4803,12 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
const uint256& txid = ptx->GetHash();
const uint256& wtxid = ptx->GetWitnessHash();
const uint256& hash = peer->m_wtxid_relay ? wtxid : txid;
AddKnownTx(*peer, hash);
- if (a
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1787644002)
Here is the change:
<details>
<summary>[patch] only remove if accepted in mempool</summary>
```diff
--- i/src/net_processing.cpp
+++ w/src/net_processing.cpp
@@ -4803,27 +4803,12 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
const uint256& txid = ptx->GetHash();
const uint256& wtxid = ptx->GetWitnessHash();
const uint256& hash = peer->m_wtxid_relay ? wtxid : txid;
AddKnownTx(*peer, hash);
- if (a
...
💬 sipa commented on issue "[Testnet] Insufficient data or no feerate found":
(https://github.com/bitcoin/bitcoin/issues/31032#issuecomment-2393598722)
Fee estimation needs data, which consists of seeing transactions arrive, and seeing them be mined. Testnet just doesn't have much data.
(https://github.com/bitcoin/bitcoin/issues/31032#issuecomment-2393598722)
Fee estimation needs data, which consists of seeing transactions arrive, and seeing them be mined. Testnet just doesn't have much data.
💬 ismaelsadeeq commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787649203)
Thanks for clarifying, initially I thought the aim is to remove the subset found by the search continuously until there are no transactions left in `m_todo`.
Maybe it will be helpful to document the point you gave as comment?
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r1787649203)
Thanks for clarifying, initially I thought the aim is to remove the subset found by the search continuously until there are no transactions left in `m_todo`.
Maybe it will be helpful to document the point you gave as comment?
💬 hebasto commented on pull request "qt6: Handle different signatures of `QANEF::nativeEventFilter`":
(https://github.com/bitcoin-core/gui/pull/840#issuecomment-2393637703)
> If Qt 6.7.0 is going to be required for Windows after the switchover; why change this code now just to immediately drop it after migrating to Qt 6?
To make https://github.com/bitcoin/bitcoin/pull/30997 focused on the build system changes only.
We already have:
```
$ git grep 'QT_VERSION_CHECK(6, 0, 0)'
src/qt/addressbookpage.cpp:#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
src/qt/addressbookpage.cpp:#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
src/qt/bitcoin.cpp:#if (QT_VERSION <
...
(https://github.com/bitcoin-core/gui/pull/840#issuecomment-2393637703)
> If Qt 6.7.0 is going to be required for Windows after the switchover; why change this code now just to immediately drop it after migrating to Qt 6?
To make https://github.com/bitcoin/bitcoin/pull/30997 focused on the build system changes only.
We already have:
```
$ git grep 'QT_VERSION_CHECK(6, 0, 0)'
src/qt/addressbookpage.cpp:#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
src/qt/addressbookpage.cpp:#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
src/qt/bitcoin.cpp:#if (QT_VERSION <
...
⚠️ vasild opened an issue: "Prioritize processing of peers based on their CPU usage"
(https://github.com/bitcoin/bitcoin/issues/31033)
### Please describe the feature you'd like to see added.
Currently, we process messages to/from all peers in a loop where every peer is processed once (has the same weight). The list of peers is shuffled before every loop.
Considering a scenario where we spend considerably more CPU time for some peers compared to others, does it make sense to de-prioritize CPU-hungry peers? This might be happening deliberately (a CPU DoS attack) or not.
For example: if we spent 5 CPU seconds to process ea
...
(https://github.com/bitcoin/bitcoin/issues/31033)
### Please describe the feature you'd like to see added.
Currently, we process messages to/from all peers in a loop where every peer is processed once (has the same weight). The list of peers is shuffled before every loop.
Considering a scenario where we spend considerably more CPU time for some peers compared to others, does it make sense to de-prioritize CPU-hungry peers? This might be happening deliberately (a CPU DoS attack) or not.
For example: if we spent 5 CPU seconds to process ea
...
💬 fanquake commented on pull request "qt6: Handle different signatures of `QANEF::nativeEventFilter`":
(https://github.com/bitcoin-core/gui/pull/840#issuecomment-2393655804)
Not sure we should be having to open and review multiple additional PRs (one now, one later to remove the code again), and be making pointless code changes, just to keep a different PR "focussed". Especially when the changes you're trying to extract are completely relevant.
(https://github.com/bitcoin-core/gui/pull/840#issuecomment-2393655804)
Not sure we should be having to open and review multiple additional PRs (one now, one later to remove the code again), and be making pointless code changes, just to keep a different PR "focussed". Especially when the changes you're trying to extract are completely relevant.
💬 willcl-ark commented on pull request "ci: set a ctest test timeout of 1200 (20 minutes)":
(https://github.com/bitcoin/bitcoin/pull/31026#issuecomment-2393726440)
> > I tried for quite some time to get this to work using `CTEST_TEST_TIMEOUT` from within _CMakeLists.txt_ but it does not appear to be possible.
>
> According to CMake docs, [`CTEST_TEST_TIMEOUT`](https://cmake.org/cmake/help/latest/variable/CTEST_TEST_TIMEOUT.html)
>
> > Specify the CTest `TimeOut` setting in a `ctest` [dashboard client script](https://cmake.org/cmake/help/latest/manual/ctest.1.html#ctest-script).
>
> which is a use case different from ours.
Huh, for some reason I
...
(https://github.com/bitcoin/bitcoin/pull/31026#issuecomment-2393726440)
> > I tried for quite some time to get this to work using `CTEST_TEST_TIMEOUT` from within _CMakeLists.txt_ but it does not appear to be possible.
>
> According to CMake docs, [`CTEST_TEST_TIMEOUT`](https://cmake.org/cmake/help/latest/variable/CTEST_TEST_TIMEOUT.html)
>
> > Specify the CTest `TimeOut` setting in a `ctest` [dashboard client script](https://cmake.org/cmake/help/latest/manual/ctest.1.html#ctest-script).
>
> which is a use case different from ours.
Huh, for some reason I
...
💬 willcl-ark commented on issue "depends: llvm-ranlib (etc): "No such file or directory" on Intel macOS 15.0":
(https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2393737848)
I also see this issue on MacOS 15.0.
> This looks odd, because you seem to be hitting code paths for macOS cross-compilation, when building natively. i.e when building on macOS, the tooling should be found by calling xcrun:
I don't think this is correct? These checks are coming from *hosts/darwin.mk* which IIUC is the correct host file to be usedwhen building on a darwin host?:
https://github.com/bitcoin/bitcoin/blob/51c698161b54402f9a8302c9a36f28a4db01fc4f/depends/hosts/darwin.mk#L19-L
...
(https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2393737848)
I also see this issue on MacOS 15.0.
> This looks odd, because you seem to be hitting code paths for macOS cross-compilation, when building natively. i.e when building on macOS, the tooling should be found by calling xcrun:
I don't think this is correct? These checks are coming from *hosts/darwin.mk* which IIUC is the correct host file to be usedwhen building on a darwin host?:
https://github.com/bitcoin/bitcoin/blob/51c698161b54402f9a8302c9a36f28a4db01fc4f/depends/hosts/darwin.mk#L19-L
...
💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2393768768)
@vasild if you rebase, tidy might point out that `sockman.cpp.o depends on i2p.cpp`. So you probably need to either move `i2p.cpp` to common as well, or remove the dependency.
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2393768768)
@vasild if you rebase, tidy might point out that `sockman.cpp.o depends on i2p.cpp`. So you probably need to either move `i2p.cpp` to common as well, or remove the dependency.