👍 fanquake approved a pull request: "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC"
(https://github.com/bitcoin/bitcoin/pull/31173#pullrequestreview-2418105101)
ACK 9e5089dbb02ef8a32f484aab682ad06748e1a532
(https://github.com/bitcoin/bitcoin/pull/31173#pullrequestreview-2418105101)
ACK 9e5089dbb02ef8a32f484aab682ad06748e1a532
💬 hebasto commented on pull request "cmake: Revamp `FindLibevent` module":
(https://github.com/bitcoin/bitcoin/pull/31181#issuecomment-2459671135)
> Can you rebase this.
Sure. Rebased.
(https://github.com/bitcoin/bitcoin/pull/31181#issuecomment-2459671135)
> Can you rebase this.
Sure. Rebased.
👍 hebasto approved a pull request: "ci: `add second_deadlock_stack=1` to TSAN options"
(https://github.com/bitcoin/bitcoin/pull/31232#pullrequestreview-2418199389)
ACK 5161c2618cd36cd2d64c64d42d6f6d3b1929cb28.
(https://github.com/bitcoin/bitcoin/pull/31232#pullrequestreview-2418199389)
ACK 5161c2618cd36cd2d64c64d42d6f6d3b1929cb28.
💬 theStack commented on pull request "tests: Handle BDB dynamic pagesize":
(https://github.com/bitcoin/bitcoin/pull/31222#issuecomment-2459691229)
Concept ACK
Note that a similar change is also included in #30125, but happy to rebase if this one goes in first.
(https://github.com/bitcoin/bitcoin/pull/31222#issuecomment-2459691229)
Concept ACK
Note that a similar change is also included in #30125, but happy to rebase if this one goes in first.
💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r1830980191)
I like simulating some time passage here.
Changing `static std::atomic<std::chrono::seconds> g_mock_time{};` to a finer precision is indeed out of the scope here.
Following a discussion on IRC I checked that indeed the time is frozen already for fuzz tests so calling `SetMockTime()` here is not going to freeze it (since it is already frozen). I added:
```cpp
SetMockTime(ConsumeTime(m_fuzzed_data_provider)); // Time could go backwards.
```
It could end up increasing the time more th
...
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r1830980191)
I like simulating some time passage here.
Changing `static std::atomic<std::chrono::seconds> g_mock_time{};` to a finer precision is indeed out of the scope here.
Following a discussion on IRC I checked that indeed the time is frozen already for fuzz tests so calling `SetMockTime()` here is not going to freeze it (since it is already frozen). I added:
```cpp
SetMockTime(ConsumeTime(m_fuzzed_data_provider)); // Time could go backwards.
```
It could end up increasing the time more th
...
💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2459694396)
`cf83f0c...c97d496`: simulate time passage and (hopefully) fix CI
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2459694396)
`cf83f0c...c97d496`: simulate time passage and (hopefully) fix CI
🤔 glozow reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2409302554)
code review ack. I still need to review tests more thoroughly. None of these comments are blocking.
Trying to summarize why these would propagate via 1p1c package relay, please correct me if I'm wrong:
- Essentially there is no longer anything "wrong" with a transaction that has 1 dust output in it since `IsStandardTx` has been changed. It's not `TX_NOT_STANDARD`. It needs to be 0 fee to pass `CheckValidEphemeralTx`, and then it should fail on feerate for `TX_RECONSIDERABLE`. That's why th
...
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2409302554)
code review ack. I still need to review tests more thoroughly. None of these comments are blocking.
Trying to summarize why these would propagate via 1p1c package relay, please correct me if I'm wrong:
- Essentially there is no longer anything "wrong" with a transaction that has 1 dust output in it since `IsStandardTx` has been changed. It's not `TX_NOT_STANDARD`. It needs to be 0 fee to pass `CheckValidEphemeralTx`, and then it should fail on feerate for `TX_RECONSIDERABLE`. That's why th
...
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830004418)
nit: const reference would be better
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830004418)
nit: const reference would be better
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825312146)
191ca0506ca69caa4d24c1f38b6f68742abb84dc nit: maybe `test_dustrelayfee_zero` or something
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1825312146)
191ca0506ca69caa4d24c1f38b6f68742abb84dc nit: maybe `test_dustrelayfee_zero` or something
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830045893)
was this supposed to spend the non-dust from dust_txid 1?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830045893)
was this supposed to spend the non-dust from dust_txid 1?
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830977092)
Noting multiple ephemeral txns spent by 1 child is allowed in this logic, but not won't propagate via opportunistic package relay. This would still be the case if we relax the child-with-unconfirmed-parents requirement because none of the parents could be submitted alone.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830977092)
Noting multiple ephemeral txns spent by 1 child is allowed in this logic, but not won't propagate via opportunistic package relay. This would still be the case if we relax the child-with-unconfirmed-parents requirement because none of the parents could be submitted alone.
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830012890)
Could speed up the common case a bit by checking `unspent_parent_dust.empty()` here, but perhaps not significantly
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830012890)
Could speed up the common case a bit by checking `unspent_parent_dust.empty()` here, but perhaps not significantly
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830037288)
Question, why minrelay instead of `DUST_RELAY_TX_FEE`?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830037288)
Question, why minrelay instead of `DUST_RELAY_TX_FEE`?
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830051037)
It also doesn't know about ancestors, given that the parent isn't given, right?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830051037)
It also doesn't know about ancestors, given that the parent isn't given, right?
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830997947)
nit: `test_no_minrelay_fee` could be better
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830997947)
nit: `test_no_minrelay_fee` could be better
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830996993)
I think "other topology" is a little under-specified here, could be helpful to name what tests you think could be added
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830996993)
I think "other topology" is a little under-specified here, could be helpful to name what tests you think could be added
🤔 hodlinator reviewed a pull request: "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC"
(https://github.com/bitcoin/bitcoin/pull/31173#pullrequestreview-2418274192)
Post-merge re-ACK 9e5089dbb02ef8a32f484aab682ad06748e1a532
Only resolving against formatting changes made to *vcpkg.json* in #31186 since my previous ACK.
(https://github.com/bitcoin/bitcoin/pull/31173#pullrequestreview-2418274192)
Post-merge re-ACK 9e5089dbb02ef8a32f484aab682ad06748e1a532
Only resolving against formatting changes made to *vcpkg.json* in #31186 since my previous ACK.
💬 maflcko commented on pull request "ci: `add second_deadlock_stack=1` to TSAN options":
(https://github.com/bitcoin/bitcoin/pull/31232#issuecomment-2459790214)
lgtm ACK 5161c2618cd36cd2d64c64d42d6f6d3b1929cb28
(https://github.com/bitcoin/bitcoin/pull/31232#issuecomment-2459790214)
lgtm ACK 5161c2618cd36cd2d64c64d42d6f6d3b1929cb28
📝 hebasto opened a pull request: "cmake: Improve robustness and usability"
(https://github.com/bitcoin/bitcoin/pull/31233)
This PR:
1. Switches to a modern CMake approach by using the `Python3::Interpreter` imported target, which is more robust than using variables.
2. Disables tests instead of ignoring them.
For example:
- building without Python available:
```
$ cmake -B build
$ cmake --build build -j 16
$ # ctest --test-dir build -j 16 -R "^util_"
Internal ctest changing into directory: /bitcoin/build
Test project /bitcoin/build
Start 113: util_tests
Start 114: util_threadnames_tests
...
(https://github.com/bitcoin/bitcoin/pull/31233)
This PR:
1. Switches to a modern CMake approach by using the `Python3::Interpreter` imported target, which is more robust than using variables.
2. Disables tests instead of ignoring them.
For example:
- building without Python available:
```
$ cmake -B build
$ cmake --build build -j 16
$ # ctest --test-dir build -j 16 -R "^util_"
Internal ctest changing into directory: /bitcoin/build
Test project /bitcoin/build
Start 113: util_tests
Start 114: util_threadnames_tests
...
👍 vasild approved a pull request: "tracing: Only prepare tracepoint arguments when actually tracing"
(https://github.com/bitcoin/bitcoin/pull/26593#pullrequestreview-2418332973)
ACK 0de3e96e333090548a43e5e870c4cb8941d6baf1
(https://github.com/bitcoin/bitcoin/pull/26593#pullrequestreview-2418332973)
ACK 0de3e96e333090548a43e5e870c4cb8941d6baf1