💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758992957)
fixed
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758992957)
fixed
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993050)
good idea, moved this subtest to `mempool_dust.py` as its own commit, switched it to testing dustrelay arg instead of standardness as I think that's more directly testing what we want.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993050)
good idea, moved this subtest to `mempool_dust.py` as its own commit, switched it to testing dustrelay arg instead of standardness as I think that's more directly testing what we want.
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993149)
dropped a note
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993149)
dropped a note
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993261)
added suggestion and a direct assertion that the main output amount is >330 the taproot dust amount
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993261)
added suggestion and a direct assertion that the main output amount is >330 the taproot dust amount
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993345)
dropped the reference to modified as it probably isn't clarifying
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993345)
dropped the reference to modified as it probably isn't clarifying
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993386)
Yes, if it's fixed this will fail, letting the author know to write a new case. Added a note that this isn't desired, just descriptive,
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993386)
Yes, if it's fixed this will fail, letting the author know to write a new case. Added a note that this isn't desired, just descriptive,
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993580)
would cause the nodes to fail to sync their mempools, no? I find it easiest when the restarts are done in sync anyways.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993580)
would cause the nodes to fail to sync their mempools, no? I find it easiest when the restarts are done in sync anyways.
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993663)
added a sentence to each saying when they should be called
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993663)
added a sentence to each saying when they should be called
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993726)
taken
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993726)
taken
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993801)
clarified comment, let me know if that's more clear
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993801)
clarified comment, let me know if that's more clear
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993882)
taken
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993882)
taken
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993968)
added a hedge word
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758993968)
added a hedge word
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758994056)
nope, changed
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1758994056)
nope, changed
🤔 pablomartin4btc reviewed a pull request: "test: Introduce ensure helper"
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2303325543)
Concept ACK
nit:
> - I have also been going back and forth on naming.
What about `ensure_during`? (another alternatives: `ensure_within_duration`/ `ensure_over_duration`) It adds a notion of time, making it more specific to the context of waiting or checking a condition over a period and it's less generic and more intuitive.
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2303325543)
Concept ACK
nit:
> - I have also been going back and forth on naming.
What about `ensure_during`? (another alternatives: `ensure_within_duration`/ `ensure_over_duration`) It adds a notion of time, making it more specific to the context of waiting or checking a condition over a period and it's less generic and more intuitive.
💬 glozow commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2349136997)
(This is a repost, sorry about that)
> It's a regression fuzz test for [#26355](https://github.com/bitcoin/bitcoin/pull/26355)
Should this fuzzer fail if I make this line return the result from IsContinuationOfLowWorkHeadersSync?
https://github.com/bitcoin/bitcoin/blob/cf0120ff024aa73a56f2975c832fda6aa8146dfa/src/net_processing.cpp#L2900
I briefly fuzzed after making the change and didn't hit anything. Likely I just didn't fuzz enough, but posting just in case somebody has a fuzz outpu
...
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2349136997)
(This is a repost, sorry about that)
> It's a regression fuzz test for [#26355](https://github.com/bitcoin/bitcoin/pull/26355)
Should this fuzzer fail if I make this line return the result from IsContinuationOfLowWorkHeadersSync?
https://github.com/bitcoin/bitcoin/blob/cf0120ff024aa73a56f2975c832fda6aa8146dfa/src/net_processing.cpp#L2900
I briefly fuzzed after making the change and didn't hit anything. Likely I just didn't fuzz enough, but posting just in case somebody has a fuzz outpu
...
👍 ryanofsky approved a pull request: "log: Use ConstevalFormatString"
(https://github.com/bitcoin/bitcoin/pull/30889#pullrequestreview-2303344498)
Code review ACK fabd78e2e30e557e04739e29c9c221ad47245df1. This is a simple change that should be useful for developers.
---
Do we know if this change affects compile time in any noticeable way? Might be worth checking since there are a lot of print statements, and compiler is now doing a little more work for each of them.
---
It might also be worth updating description to give a sense of what errors look like. For me they look like:
```c++
src/util/string.h: In function ‘int main
...
(https://github.com/bitcoin/bitcoin/pull/30889#pullrequestreview-2303344498)
Code review ACK fabd78e2e30e557e04739e29c9c221ad47245df1. This is a simple change that should be useful for developers.
---
Do we know if this change affects compile time in any noticeable way? Might be worth checking since there are a lot of print statements, and compiler is now doing a little more work for each of them.
---
It might also be worth updating description to give a sense of what errors look like. For me they look like:
```c++
src/util/string.h: In function ‘int main
...
💬 jonatack commented on pull request "doc: unit test runner help fixup and improvements":
(https://github.com/bitcoin/bitcoin/pull/30890#issuecomment-2349148563)
Thanks everyone, updated to take all feedback.
(https://github.com/bitcoin/bitcoin/pull/30890#issuecomment-2349148563)
Thanks everyone, updated to take all feedback.
👍 tdb3 approved a pull request: "doc: unit test runner help fixup and improvements"
(https://github.com/bitcoin/bitcoin/pull/30890#pullrequestreview-2303356646)
re ACK 44caadd83dbc612f41c70baaea3055c52f27df90
(https://github.com/bitcoin/bitcoin/pull/30890#pullrequestreview-2303356646)
re ACK 44caadd83dbc612f41c70baaea3055c52f27df90
💬 jonatack commented on pull request "doc: unit test runner help fixup and improvements":
(https://github.com/bitcoin/bitcoin/pull/30890#issuecomment-2349155516)
(Thanks @tdb3, repushed to put the text suggested by @pablomartin4btc in the right place.)
(https://github.com/bitcoin/bitcoin/pull/30890#issuecomment-2349155516)
(Thanks @tdb3, repushed to put the text suggested by @pablomartin4btc in the right place.)
👍 itornaza approved a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2303370741)
ACK b95bb2179610183d9398d50d8c8fd24b1450ad4d
The changes are on `src/ipc/capnp/common-types.h` and greatly enhance code reuse, readability and clarity through the use of concepts. As per the standard review procedure, I rebuilt and run all the tests locally and all of them pass.
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2303370741)
ACK b95bb2179610183d9398d50d8c8fd24b1450ad4d
The changes are on `src/ipc/capnp/common-types.h` and greatly enhance code reuse, readability and clarity through the use of concepts. As per the standard review procedure, I rebuilt and run all the tests locally and all of them pass.