🤔 rkrux reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2417722869)
Successful make and functional tests at 131bed19bdfc922328cad9781fa9122b6810a8fd.
I've gone through only the src/policy/*, src/rpc/*, src/validation.cpp yet. My comments are mostly around code structure. I plan to review the functional tests and src/tests/* very soon.
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2417722869)
Successful make and functional tests at 131bed19bdfc922328cad9781fa9122b6810a8fd.
I've gone through only the src/policy/*, src/rpc/*, src/validation.cpp yet. My comments are mostly around code structure. I plan to review the functional tests and src/tests/* very soon.
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830689563)
Nit: This function is independent of the ephemeral nature and is generic enough to lie outside this file, near where `IsDust()` is.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830689563)
Nit: This function is independent of the ephemeral nature and is generic enough to lie outside this file, near where `IsDust()` is.
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830700460)
Although it seems correct to run the loop for the outputs of the parent transaction but as per the definition of Ephemeral Dust, we can break (and end) the loop here after the dust is found because only one is allowed per parent transaction?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830700460)
Although it seems correct to run the loop for the outputs of the parent transaction but as per the definition of Ephemeral Dust, we can break (and end) the loop here after the dust is found because only one is allowed per parent transaction?
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830720982)
Overall this is a pretty neat function that I enjoyed going through!
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830720982)
Overall this is a pretty neat function that I enjoyed going through!
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830693824)
> Ephemeral dust is a new concept that allows a single
dust output in a transaction, provided the transaction
is zero fee.
As per this definition, shouldn't this function also check that there is only one dust output in the transaction? I can see it's checked in the `IsStandardTx()` but IMO this function that should check the complete validity of the ephemeral transaction should encapsulate this single dust output check as well.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830693824)
> Ephemeral dust is a new concept that allows a single
dust output in a transaction, provided the transaction
is zero fee.
As per this definition, shouldn't this function also check that there is only one dust output in the transaction? I can see it's checked in the `IsStandardTx()` but IMO this function that should check the complete validity of the ephemeral transaction should encapsulate this single dust output check as well.
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830774549)
With this change, `IsStandardTx()` allows having only 1 dust output per tx and makes such txs technically standard but without any constraint of the 0-fee part that comes later down in the `PreChecks()`. Although there are no other usages of the `IsStandardTx()` in the source code (besides the CPP tests) but if later usages do arise, they would miss out on the 0-fee check.
TL;DR: The 0-fee check and the 1-dust output check that are tied by the Ephemeral Dust concept don't exist together within
...
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830774549)
With this change, `IsStandardTx()` allows having only 1 dust output per tx and makes such txs technically standard but without any constraint of the 0-fee part that comes later down in the `PreChecks()`. Although there are no other usages of the `IsStandardTx()` in the source code (besides the CPP tests) but if later usages do arise, they would miss out on the 0-fee check.
TL;DR: The 0-fee check and the 1-dust output check that are tied by the Ephemeral Dust concept don't exist together within
...
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830784855)
> Priority is not supported for transactions with dust outputs.
Nit: IIUC, there should not be more than 1 dust output per transaction but this reads otherwise.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830784855)
> Priority is not supported for transactions with dust outputs.
Nit: IIUC, there should not be more than 1 dust output per transaction but this reads otherwise.
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830778947)
There is a underlying assumption here that this constant is used only for the ephemeral dust use case but it is not evident in the naming of this constant. Related to the previous comment on `IsStandardTx()`.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830778947)
There is a underlying assumption here that this constant is used only for the ephemeral dust use case but it is not evident in the naming of this constant. Related to the previous comment on `IsStandardTx()`.
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830898538)
> Must be called for each transaction(package) if any dust is in the package. Checks that each transaction's parents have their dust spent by the child,
Should this loop run only for dust inputs by adding a `IsDust()` right at the beginning of this loop? I'm assuming this function is not responsible for the standard transaction validity checks as per the comment in the header file.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830898538)
> Must be called for each transaction(package) if any dust is in the package. Checks that each transaction's parents have their dust spent by the child,
Should this loop run only for dust inputs by adding a `IsDust()` right at the beginning of this loop? I'm assuming this function is not responsible for the standard transaction validity checks as per the comment in the header file.
💬 glozow commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2459565875)
@dergoegge is that on master, or with the changes in this PR? Would like to see if it could be a negative time thing.
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2459565875)
@dergoegge is that on master, or with the changes in this PR? Would like to see if it could be a negative time thing.
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830902499)
Aah nvm, `IsDust` works on TxOuts.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830902499)
Aah nvm, `IsDust` works on TxOuts.
💬 dergoegge commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2459568882)
> is that on master
Sorry I should have clarified, it is on master.
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2459568882)
> is that on master
Sorry I should have clarified, it is on master.
💬 fanquake commented on pull request "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC":
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2459572965)
Guix Build:
```bash
ea6e6faf76005701a3251872e64ee1dd5c7e1222d28b38a273d9c1851470e927 guix-build-9e5089dbb02e/output/aarch64-linux-gnu/SHA256SUMS.part
b71cc1acf87a5394334149a2cd724e6acd97b2660d7f74d32b97e62f83a539d6 guix-build-9e5089dbb02e/output/aarch64-linux-gnu/bitcoin-9e5089dbb02e-aarch64-linux-gnu-debug.tar.gz
d4879760d76da7aa4a41fe56981a0c2992032ea41e9460ef60036dc57599e9e7 guix-build-9e5089dbb02e/output/aarch64-linux-gnu/bitcoin-9e5089dbb02e-aarch64-linux-gnu.tar.gz
2cbd20b0b10f97fb
...
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2459572965)
Guix Build:
```bash
ea6e6faf76005701a3251872e64ee1dd5c7e1222d28b38a273d9c1851470e927 guix-build-9e5089dbb02e/output/aarch64-linux-gnu/SHA256SUMS.part
b71cc1acf87a5394334149a2cd724e6acd97b2660d7f74d32b97e62f83a539d6 guix-build-9e5089dbb02e/output/aarch64-linux-gnu/bitcoin-9e5089dbb02e-aarch64-linux-gnu-debug.tar.gz
d4879760d76da7aa4a41fe56981a0c2992032ea41e9460ef60036dc57599e9e7 guix-build-9e5089dbb02e/output/aarch64-linux-gnu/bitcoin-9e5089dbb02e-aarch64-linux-gnu.tar.gz
2cbd20b0b10f97fb
...
💬 glozow commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2459581052)
Thanks @dergoegge, I probably also could have inferred from it being oss-fuzz.
Even though I wrote it, the assertion looks wrong 😅 overloaded inflight is handled with just a delay
https://github.com/bitcoin/bitcoin/blob/80cb630bd945e7f1777c7bab041c411b6916e651/src/node/txdownloadman_impl.cpp#L199
I think what I was trying to do was ensure `MAX_PEER_TX_ANNOUNCEMENTS` isn't busted. Adding a fix here.
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2459581052)
Thanks @dergoegge, I probably also could have inferred from it being oss-fuzz.
Even though I wrote it, the assertion looks wrong 😅 overloaded inflight is handled with just a delay
https://github.com/bitcoin/bitcoin/blob/80cb630bd945e7f1777c7bab041c411b6916e651/src/node/txdownloadman_impl.cpp#L199
I think what I was trying to do was ensure `MAX_PEER_TX_ANNOUNCEMENTS` isn't busted. Adding a fix here.
👍 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