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

💬 ZKcash-IrishGov commented on something "":
(https://github.com/bitcoin/bitcoin/commit/0328dcdcfcb56dc8918697716d7686be048ad0b3#r149113901)
https://explorer.electriccash.global/

👍 ryanofsky approved a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2437555054)
Code review ACK 2c15a858ca7b3f14eaebf4b39134cd0e9c559273. No changes since the last review other than rebase.

I do think it would simplify this PR and make review more meaningful if #31072 could be merged first. This PR is combining application code changes which require looking at surrounding code and seeing if the changes fit in, with translation class changes which require thinking about API design. I think it's harder to evaluate both things clearly when they are bundled in the same PR. I
...

💬 luke-jr commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1843126749)
Seems trivial to fix properly? https://github.com/luke-jr/bitcoin/commit/56981318da5dc3145e9f244ad0584ef05c815f6b

💬 luke-jr commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2477853828)
Any reason not to scope the new variables? https://github.com/luke-jr/bitcoin/commit/851d354fc4f94695aabd12d21e470dc90d267b93

👍 tdb3 approved a pull request: "refactor: Avoid std::string format strings"
(https://github.com/bitcoin/bitcoin/pull/31287#pullrequestreview-2437567836)
code review and light test ACK fa1177e3d7ca9ef003ded4d0c915fa6dc22bd37d

Quick checks:
- Viewed the `-printpriority` description with `bitcoind -help -help-debug`
- Induced test failure in `txrequest_tests` to see the string.

💬 ryanofsky commented on pull request "refactor: Clean up messy strformat and bilingual_str usages":
(https://github.com/bitcoin/bitcoin/pull/31072#issuecomment-2477861319)
Updated 46c55ab427c0771cacaed7a20612ffe2cc3543db -> 1e9ea7de0b5ee6d3179930d4fc93c8e276e525bd ([`pr/bfmt.3`](https://github.com/ryanofsky/bitcoin/commits/pr/bfmt.3) -> [`pr/bfmt.4`](https://github.com/ryanofsky/bitcoin/commits/pr/bfmt.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bfmt.3..pr/bfmt.4)) with no changes. Just updated commit messages to be more accurate now that #31174 is merged.

💬 ryanofsky commented on pull request "ci: skip Github CI on branch pushes for forks":
(https://github.com/bitcoin/bitcoin/pull/30487#issuecomment-2477864531)
> Just repeating my "note to maintainers" from the description: like with Cirrus, `SKIP_BRANCH_PUSH=true` needs to be set for the GUI repo to maintain existing behaviour.
>
> Probably [here](https://github.com/bitcoin-core/gui/settings/variables/actions).

Can confirm this was set at the time this PR was merged.