💬 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

💬 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

💬 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,

💬 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.

💬 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

💬 instagibbs commented on pull request "Ephemeral Dust":
(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

💬 instagibbs commented on pull request "Ephemeral Dust":
(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

💬 instagibbs commented on pull request "Ephemeral Dust":
(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.

💬 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
...

👍 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
...

💬 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.

👍 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

💬 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.)

👍 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.

💬 mzumsande commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1759034089)
I agree that it shouldn't be possible to have undo data but no block data.
On the other hand, callers of this function may not even be interested in the block data, and I'm not sure if a RPC call is the right place to check for this inconsistency. At least it's checked in `CheckBlockIndex()` [here](https://github.com/bitcoin/bitcoin/blob/06a9f7789e359131abad2489a221888f711bdc3b/src/validation.cpp#L5326), so I think I'll leave this as is for now.

💬 mzumsande commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1759034840)
fixed

💬 mzumsande commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#issuecomment-2349173516)
[a5df908 ](https://github.com/bitcoin/bitcoin/commit/a5df908d981138f066f5f963a1813d453483814a)to [6a1aa51](https://github.com/bitcoin/bitcoin/commit/6a1aa510e31e8b77793341473aa5afc9d023a6e3): Fixed typos.