💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1967597634)
Done.

💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1967598306)
The commit message was outdated, I just pushed fixing it, thanks for noticing!

💬 ryanofsky commented on issue "intermittent ipc_tests (Timeout)":
(https://github.com/bitcoin/bitcoin/issues/31921#issuecomment-2678349700)
> Might be best carve out the bugfix commit for now.

Thanks I moved the commit bumping libmultiprocess version from #31741 to a new PR #31945

💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2678351673)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1966518694 and https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1967529542. Fixed a outdated commit message.

💬 maflcko commented on issue "intermittent issue in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2678376360)
Another from a local run: https://drahtbot.space/temp_scratch/p2p_o_h_208.tar.zstd

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2678376632)
Rebased 828c440b0dea29ab41ffc32d88969176d1286655 -> da25311e3a88941bf99a732be1aa3568a65f2273 ([`pr/subtree.18`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.18) -> [`pr/subtree.19`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.19), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/subtree.18-rebase..pr/subtree.19)) due to conflict with #31662

💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2678395237)
Rebased 59e1d4abb34453805a09a62c51ead7347787b2e3 -> b4f024ea336f72fd1023d0929761978566126740 ([`pr/mine.17`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.17) -> [`pr/mine.18`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.18), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.17-rebase..pr/mine.18)) due to conflict with #31865

💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967626224)
I don't think the other asserts are any different. For example, `LLVM_PROFDATA` may fail, because the executable was not compiled with coverage:

```
error: test_det_cov.profraw: No such file or directory
thread 'main' panicked at src/main.rs:80:9:
assertion failed: Command::new(LLVM_PROFDATA).arg("merge").arg("--sparse").arg(&profraw_file).arg("-o").arg(&profdata_file).status().expect("merge failed").success()
stack backtrace:
...
```

Similarly, as already mentioned, the `expect("tes
...

👍 rkrux approved a pull request: "descriptor: check whitespace in keys within fragments"
(https://github.com/bitcoin/bitcoin/pull/31603#pullrequestreview-2637096822)
tACK 21e9d39a3725cd6107b742f0cb97f65b3640201b

Thanks for addressing the feedback.

💬 hodlinator commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967644930)
> No strong opinion, but I think this should either be done for all places, or for none.

Note that you are using `exit(1)` for some cases, so you are already drawing an arbitrary line.

A user using something like `miniscript` as input is more likely than an permission failure IMO. Agree that not compiling with coverage is somewhere in between though.

If we're going to introduce more Rust code in the project, and use it in an interactive way, I think we should spend some time to make the
...

💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967697007)
I don't think this has something to do with the language it is written in. I am sure it is possible to write a check for the exit code in any language and print a different error message than an assertion failure.



> Note that you are using `exit(1)` for some cases, so you are already drawing an arbitrary line.

Yes, currently the line here is for any sanity check that can be cheaply done while parsing the args and print the help on failure.

I understand your concern, and I'd be happy
...

💬 maflcko commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2678514099)
The first commit is split up in https://github.com/bitcoin/bitcoin/pull/31945 and I tagged it for 29.0 as a bugfix. Thus, reviewers of this pull may want to review https://github.com/bitcoin/bitcoin/pull/31945 first.

💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2678556082)
CI failure is unrelated.

💬 brunoerg commented on issue "test: intermittent issue in p2p_1p1c_network.py ":
(https://github.com/bitcoin/bitcoin/issues/31721#issuecomment-2678560262)
https://github.com/bitcoin/bitcoin/actions/runs/13498602529/job/37711361328?pr=31603#step:6:3286

🤔 hodlinator reviewed a pull request: "test: create assert_not_equal util"
(https://github.com/bitcoin/bitcoin/pull/29500#pullrequestreview-2637139931)
Concept ACK d9cb032ec1aa7000fc3f5b9fa48405f269879497

💬 hodlinator commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r1967667753)
nit: Should appear after `assert_equal` alphabetically.

Same in test/functional/feature_dbcrash.py and others.

💬 hodlinator commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r1967712185)
Agree this is a valid argument against the PR. It is very seldom useful to print the values when they are equal.

I still prefer `assert_not_equal()` for:
* Symmetry with `assert_equal()`.
* Distinguishing itself from `assert` which is skipped when running Python with `-O`.

💬 hodlinator commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r1967695087)
nit: Feels a bit weird to not have `assert_equal()` here and other directly adjacent places. Could touch up adjacent lines like this in separate commit?

💬 hodlinator commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r1967685068)
Should use f-strings:
https://github.com/bitcoin/bitcoin/blob/e486597f9a57903600656fb5106858941885852f/test/functional/README.md?plain=1#L40

nit: And possibly rephrase:
```suggestion
raise AssertionError(f"Both values are {thing1}{f', {error_message}' if error_message else ''}")
```

💬 l0rinc commented on pull request "log,optimization: use original log string when no suspicious chars found":
(https://github.com/bitcoin/bitcoin/pull/31923#discussion_r1967752283)
Thanks for the review!
The benchmarks indicate that this potential extra copy doesn't matter, but we can of course move the short-circuiting to the call site, that should make this more intuitive (we're still getting one copy now, but at least it's not character-by-character).
What do you think, is this better?