💬 achow101 commented on pull request "Add `contrib/justfile` containing useful development workflow commands.":
(https://github.com/bitcoin/bitcoin/pull/31292#issuecomment-2477456396)
Concept NACK

We generally do not include files that are specific to particular people's development environments such as IDE configurations, and this seems to be pretty close to that kind of thing. Nor do we add to our gitignore files that are specific to particular people's workflows as those should be part of their global .gitignore.

🚀 achow101 merged a pull request: "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues"
(https://github.com/bitcoin/bitcoin/pull/30349)

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

> Groups all tests executed within each binary call under a single directory prefixed by the current time. Replicating the function test framework behavior.

It makes sense to me. I'll play a bit with it just to see how it looks.

💬 casey commented on pull request "Add `contrib/justfile` containing useful development workflow commands.":
(https://github.com/bitcoin/bitcoin/pull/31292#issuecomment-2477461070)
> Concept NACK
>
> We generally do not include files that are specific to particular people's development environments such as IDE configurations, and this seems to be pretty close to that kind of thing. Nor do we add to our gitignore files that are specific to particular people's workflows as those should be part of their global .gitignore.

Fair enough! I will say that I think this is generally useful to anyone who works in the terminal, and serves as a generally useful source of document
...

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