🤔 hebasto reviewed a pull request: "refactor: move util/pcp and util/netif to common/"
(https://github.com/bitcoin/bitcoin/pull/31011#pullrequestreview-2347681620)
Post-merge ACK fd38711217cafbd62e8abd22d2b43f85fede8cde.
(https://github.com/bitcoin/bitcoin/pull/31011#pullrequestreview-2347681620)
Post-merge ACK fd38711217cafbd62e8abd22d2b43f85fede8cde.
🚀 hebasto merged a pull request: "qt6: Fix linking when configured with `-DENABLE_WALLET=OFF`"
(https://github.com/bitcoin-core/gui/pull/837)
(https://github.com/bitcoin-core/gui/pull/837)
💬 promag commented on pull request "cmake: Avoid hardcoding Qt's major version in Find module / variable names":
(https://github.com/bitcoin/bitcoin/pull/31010#issuecomment-2393380465)
Code review ACK deacf3c7cd68dd4ee973526740e45c7015b6433a.
(https://github.com/bitcoin/bitcoin/pull/31010#issuecomment-2393380465)
Code review ACK deacf3c7cd68dd4ee973526740e45c7015b6433a.
💬 marcofleon commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2393409008)
@instagibbs thanks, that makes sense. I did see the `fRejectedParents` is true case with an input that crashed on 3c1c9e766ad4fc6defdc9b4814c1e184f6603003 (before the last push).
Option 1 in your list seems most robust to me as well.
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2393409008)
@instagibbs thanks, that makes sense. I did see the `fRejectedParents` is true case with an input that crashed on 3c1c9e766ad4fc6defdc9b4814c1e184f6603003 (before the last push).
Option 1 in your list seems most robust to me as well.
🤔 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
...