💬 maflcko commented on pull request "ci: honor ci bypass prefix in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2349100068)
1 commit every 4 years, or even 1 commit every year that qualifies for this just doesn't seem worth it the overhead.

If you want to break `git bisect`, there should be a good reason. A reason well enough that reviewers run the appropriate `git rebase --interactive --exec` locally. If there is no good enough reason, then the breaking change probably shouldn't be done.

💬 Sjors commented on pull request "ci: honor ci bypass prefix in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2349119854)
> A reason well enough that reviewers run the appropriate git `rebase --interactive --exec` locally.

That seems a fairly arbitrary metric for measuring how good a reason is.

I'm more sympathetic to the possibility that both the reviewers and maintainer overlook a `[skip ci]` commit message (especially if it's not on the first line), causing `git bisect` to break. And worse, potentially allowing to sneak in a non-refactoring change.

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

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

💬 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