💬 hodlinator commented on pull request "test: Fix RANDOM_CTX_SEED use with parallel tests":
(https://github.com/bitcoin/bitcoin/pull/30737#issuecomment-2477467410)
> I think the issue in the fuzz tests still persists.

That's correct. Fuzzing was not my main concern, but could maybe resurrect that part of this PR.

I've rebased on current master and re-pushed the branch I had deleted, but am not allowed to reopen this one.
https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:2024-08/RANDOM_CTX_SEED_jobs_fix_alt

Are you still against passing through RANDOM_CTX_SEED in the test_runner.py? In that case I can drop that commit.

💬 achow101 commented on pull request "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment":
(https://github.com/bitcoin/bitcoin/pull/30666#issuecomment-2477470155)
ACK 0bd53d913c1c2ffd2d0779f01bc51c81537b6992

🚀 achow101 merged a pull request: "ci: skip Github CI on branch pushes for forks"
(https://github.com/bitcoin/bitcoin/pull/30487)

💬 luke-jr commented on pull request "rpc: add optional blockhash to waitfornewblock":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r1842929586)
Why do all this? Can't we just compare `hash` to `block` and return?

🤔 ismaelsadeeq reviewed a pull request: "Fee Estimation via Fee rate Forecasters"
(https://github.com/bitcoin/bitcoin/pull/30157#pullrequestreview-2437270515)
Thanks for your conceptual review, @murchandamus and @vasild. There's also another review by @remyers in the tracking issue: https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2410695144.

I have updated this PR based on feedback from #30391 and in-person conversations:

1. I am now using the newly introduced `FeeFrac` datatype, which does not lose precision like `CFeeRate`.
2. Updated the `CalculatePercentile` function to return a monotonically decreasing estimate for high and lo
...

✅ achow101 closed an issue: "Header inconsistency after invalidate/reconsider block"
(https://github.com/bitcoin/bitcoin/issues/26245)

🚀 achow101 merged a pull request: "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment"
(https://github.com/bitcoin/bitcoin/pull/30666)

💬 ismaelsadeeq commented on pull request "BlockAssembler: return selected packages virtual size and fee":
(https://github.com/bitcoin/bitcoin/pull/30391#issuecomment-2477493180)
> So there are clients that this could break.

@Sjors, could you clarify your coment?
Which clients would this affect?

💬 hodlinator commented on pull request "test: add global time to place exec tests within the same dir":
(https://github.com/bitcoin/bitcoin/pull/31291#discussion_r1842931899)
Global mutable state should be avoided if possible, just for peace of mind.
```suggestion
static const uint64_t g_start_time = TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now());
```

🤔 hodlinator reviewed a pull request: "test: add global time to place exec tests within the same dir"
(https://github.com/bitcoin/bitcoin/pull/31291#pullrequestreview-2437271418)
Concept ACK ce0645c312d0395627f1497140108cfe91473bc2

Only case I can think of where having *time* segment before `test_name` in the path would be annoying is if one was re-running 2 specific tests a variable amount of times. This PR makes it so that one has to "open every door" (*time* segment) to see if files for test-A was there or for test-B. In general I think this way is a net-positive though.

Good to see the `-testdatadir` description updated, sorry for not catching it in #31000.


...

💬 hodlinator commented on pull request "test: add global time to place exec tests within the same dir":
(https://github.com/bitcoin/bitcoin/pull/31291#discussion_r1842945708)
nit: We are recalculating `util::ToString()` for the same value every time we pass by this. Could do it once on line 80.
```suggestion
m_path_root = fs::temp_directory_path() / TEST_DIR_PATH_ELEMENT / g_start_time / test_name;
```

💬 furszy commented on pull request "test: add global time to place exec tests within the same dir":
(https://github.com/bitcoin/bitcoin/pull/31291#issuecomment-2477553548)
> Only case I can think of where having time segment before test_name in the path would be annoying is if one was re-running 2 specific tests a variable amount of times.

For that we could introduce a `-repeat` arg, and suffix the dir with the loop round number.

💬 hodlinator commented on pull request "test: add global time to place exec tests within the same dir":
(https://github.com/bitcoin/bitcoin/pull/31291#issuecomment-2477559856)
> > Only case I can think of where having time segment before test_name in the path would be annoying is if one was re-running 2 specific tests a variable amount of times.
>
> For that we could introduce a `-repeat` arg, and suffix the dir with the loop round number.

Maybe at that stage it's acceptable to require unique `-testdatadir` being passed in for every run.

💬 furszy commented on pull request "test: add global time to place exec tests within the same dir":
(https://github.com/bitcoin/bitcoin/pull/31291#discussion_r1842979168)
Sure. Done as suggested.

💬 furszy commented on pull request "test: add global time to place exec tests within the same dir":
(https://github.com/bitcoin/bitcoin/pull/31291#discussion_r1842979267)
Sure. Done as suggested. Thanks.

💬 furszy commented on pull request "test: add global time to place exec tests within the same dir":
(https://github.com/bitcoin/bitcoin/pull/31291#issuecomment-2477562013)
> > > Only case I can think of where having time segment before test_name in the path would be annoying is if one was re-running 2 specific tests a variable amount of times.
> >
> >
> > For that we could introduce a `-repeat` arg, and suffix the dir with the loop round number.
>
> Maybe at that stage it's acceptable to require unique `-testdatadir` being passed in for every run.

For sure. That wouldn't be hard to do neither.

👍 hodlinator approved a pull request: "test: add global time to place exec tests within the same dir"
(https://github.com/bitcoin/bitcoin/pull/31291#pullrequestreview-2437371463)
ACK b7f40e8094e00c13a6c847340645b16704fed63b

Ran `ctest -j 20 --test-dir build` and `build/src/test/test_bitcoin` and inspected activity with `lsof |grep "/tmp.*bitcoin`. The former will create a new process and hence *time* directory for every test, but run them in parallel. The latter will run all tests sequentially in-process, resulting in only one new *time* directory.

💬 hodlinator commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2477602934)
@ryanofsky thanks for the review! Updated to cover negation for all users of `AbsPathForConfigVal`.

👍 ryanofsky approved a pull request: "Add destroy to BlockTemplate schema"
(https://github.com/bitcoin/bitcoin/pull/31288#pullrequestreview-2437526746)
Code review ACK 5f7dc0dbe64eb38afc74e9c4cba489382ea373bd

> @ryanofsky can you further clarify what the difference in behavior is with an without a `destroy` method?

I think it probably doesn't matter too much in this case, since the only thing the destroy method will currently do is free memory. But this does help future proof the interface, in case we want the destructor to do more things in the future, or if we add memory limits and want to give clients a way to ensure that previous bloc
...

💬 ryanofsky commented on pull request "Add destroy to BlockTemplate schema":
(https://github.com/bitcoin/bitcoin/pull/31288#discussion_r1843099829)
In commit "Add destroy to BlockTemplate schema" (5f7dc0dbe64eb38afc74e9c4cba489382ea373bd)

Would suggest making destroy the first method because it affect semantics of the object as a whole, and thats where I placed destroy in all the other interfaces. It is fine to keep the number @9 for it, or renumber if you prefer.