💬 promag commented on pull request "qt6: Fix linking when configured with `-DENABLE_WALLET=OFF`":
(https://github.com/bitcoin-core/gui/pull/837#issuecomment-2393139603)
ACK 5be34bacf6d07fb73a0cedfb63a384968d538f3e, all other changes in 33657e1c958146312e4c68765a92871920401396 are not required, makes the code harder to read.
(https://github.com/bitcoin-core/gui/pull/837#issuecomment-2393139603)
ACK 5be34bacf6d07fb73a0cedfb63a384968d538f3e, all other changes in 33657e1c958146312e4c68765a92871920401396 are not required, makes the code harder to read.
💬 hebasto commented on pull request "qt6: Fix linking when configured with `-DENABLE_WALLET=OFF`":
(https://github.com/bitcoin-core/gui/pull/837#discussion_r1787391324)
> Reworked.
Reverted back.
> How the `RPCExecutor::request` method compiles when this line doesn't exist?
Due to https://github.com/bitcoin-core/gui/blob/5be34bacf6d07fb73a0cedfb63a384968d538f3e/src/qt/rpcconsole.h#L24
(https://github.com/bitcoin-core/gui/pull/837#discussion_r1787391324)
> Reworked.
Reverted back.
> How the `RPCExecutor::request` method compiles when this line doesn't exist?
Due to https://github.com/bitcoin-core/gui/blob/5be34bacf6d07fb73a0cedfb63a384968d538f3e/src/qt/rpcconsole.h#L24
🚀 hebasto merged a pull request: "qt6, test: Handle deprecated code"
(https://github.com/bitcoin-core/gui/pull/839)
(https://github.com/bitcoin-core/gui/pull/839)
💬 itornaza 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-2393255981)
Same error messages as @Sjors on a clean build as well.
```
ioannis@ergot_a depends % make
/bin/sh: command -v llvm-ranlib: No such file or directory
/bin/sh: command -v llvm-strip: No such file or directory
/bin/sh: command -v llvm-nm: No such file or directory
/bin/sh: command -v llvm-objdump: No such file or directory
/bin/sh: command -v dsymutil: No such file or directory
Fetching boost_1_81_0.tar.gz from https://archives.boost.io/release/1.81.0/source/
% Total % Received % X
...
(https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2393255981)
Same error messages as @Sjors on a clean build as well.
```
ioannis@ergot_a depends % make
/bin/sh: command -v llvm-ranlib: No such file or directory
/bin/sh: command -v llvm-strip: No such file or directory
/bin/sh: command -v llvm-nm: No such file or directory
/bin/sh: command -v llvm-objdump: No such file or directory
/bin/sh: command -v dsymutil: No such file or directory
Fetching boost_1_81_0.tar.gz from https://archives.boost.io/release/1.81.0/source/
% Total % Received % X
...
💬 Sjors commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1787471881)
> The code should just be able to call `waitTipChanged()` with the zero value, which will return as soon as node is started and it knows what the tip is.
That indeed seems like a better approach, will change that for the TP.
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1787471881)
> The code should just be able to call `waitTipChanged()` with the zero value, which will return as soon as node is started and it knows what the tip is.
That indeed seems like a better approach, will change that for the TP.
💬 Sjors commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#issuecomment-2393327733)
ACK fa22e5c430acaef9713d9a4b4b97bb3f4876f816
Manually tested `-stopatheight`, a fatal indexer error (by breaking some code), calling `bitcoin-cli stop`, manually quitting qt, calling `stop` form the GUI console.
(https://github.com/bitcoin/bitcoin/pull/30967#issuecomment-2393327733)
ACK fa22e5c430acaef9713d9a4b4b97bb3f4876f816
Manually tested `-stopatheight`, a fatal indexer error (by breaking some code), calling `bitcoin-cli stop`, manually quitting qt, calling `stop` form the GUI console.
💬 hebasto commented on issue "Unable to cross compile on linux for macos (28.x branch)":
(https://github.com/bitcoin/bitcoin/issues/31027#issuecomment-2393328681)
> debian unstable (sid) runs on clang-20 doesn't need to downgrade it
Debian Sid's [default clang](https://packages.debian.org/sid/clang) is clang-16, which is enough to build every dependency except for the `qt` package.
Here is a full log of the commands on a fresh Debian Sid image:
```
# history
1 apt update
2 apt install git bison cmake curl make pkg-config xz-utils wget
3 apt install clang llvm lld
4 git clone https://github.com/bitcoin/bitcoin.git
5 w
...
(https://github.com/bitcoin/bitcoin/issues/31027#issuecomment-2393328681)
> debian unstable (sid) runs on clang-20 doesn't need to downgrade it
Debian Sid's [default clang](https://packages.debian.org/sid/clang) is clang-16, which is enough to build every dependency except for the `qt` package.
Here is a full log of the commands on a fresh Debian Sid image:
```
# history
1 apt update
2 apt install git bison cmake curl make pkg-config xz-utils wget
3 apt install clang llvm lld
4 git clone https://github.com/bitcoin/bitcoin.git
5 w
...
👍 Sjors approved a pull request: "refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2347659011)
ACK fa22e5c430acaef9713d9a4b4b97bb3f4876f816
Manually tested `-stopatheight`, a fatal indexer error (by breaking some code), calling `bitcoin-cli stop`, manually quitting qt, calling `stop` form the GUI console.
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2347659011)
ACK fa22e5c430acaef9713d9a4b4b97bb3f4876f816
Manually tested `-stopatheight`, a fatal indexer error (by breaking some code), calling `bitcoin-cli stop`, manually quitting qt, calling `stop` form the GUI console.
🤔 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
...