💬 mzumsande commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2361266204)
Would be nice to respond to my prior message: Why doesn't your [Miner log](https://github.com/braidpool/bitcoin/issues/1#issuecomment-2341397203) show a timestamped "[Calling bitcoin-cli](https://github.com/braidpool/bitcoin/blob/29c38babfb83f8520a882bad6b25d1851b7b4a41/contrib/cpunet/miner#L108) (...) getblocktemplate" entry, if, supposedly, the relevant call to `getblocktemplate` call was sent off after the zmq notification for the new tip was received?

Without an explanation for that, it j
...

👍 tdb3 approved a pull request: "Follow-up after AutoFile position caching: remove unused code"
(https://github.com/bitcoin/bitcoin/pull/30927#pullrequestreview-2315834850)
CR ACK caac06f784c5d94c6a5f7d0b586f9ddbbe55c369

💬 maflcko commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767007551)
Not sure about this. One is unconditionally logging and the other not. That is a pretty stark difference and would be better to leave on separate lines. In any case, this isn't a refactor, because you drop the trailing `\n`, and it isn't needed for the changes here, so it may be best to drop.

💬 l0rinc commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767001843)
Since we have simple, atomic instructions in both branches after which we immediately return, we might as well return directly:
```suggestion
try {
return tfm::format(fmt.fmt, args...);
} catch (tinyformat::format_error& fmterr) {
/* Original format string will have newline so don't add one here */
return "Error \""s + fmterr.what() + "\" while formatting log message: \"" + fmt.fmt + "\"";
}
```

Note: I've used `using namespace std::string_literals`
...

💬 l0rinc commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767010840)
comment seems redundant here

💬 l0rinc commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767009245)
Is the `LogFlags::NONE` -> `BCLog::NET` category in case of `manual_connection` a behavior change?

💬 theStack commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2361325488)
Rebased on master and adapted the `dumptxoutset` call in the test to the new interface (see #29553).

✅ mcelrath closed an issue: "Race condition between ZMQ UpdateTip and getblocktemplate"
(https://github.com/bitcoin/bitcoin/issues/30862)

💬 mcelrath commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2361325732)
@mzumsande you are right. Apparently I failed to scroll up in the log. There was several tips announced simultaneously and this is absolutely due to a previous invocation of `getblocktemplate`. I'm going to close issue this now, as I've also failed to reproduce it since.

Log with more context attached.

[miner.log](https://github.com/user-attachments/files/17061890/miner.log)

💬 stickies-v commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767048974)
Thanks for pointing out the behaviour change, I've dropped the commit.

💬 stickies-v commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767049495)
Taken (minus string literals), thanks!

💬 stickies-v commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767050387)
It is indeed, thanks for pointing it out. Resolved by dropping the first commit and applying a minimal diff to `netbase.cpp`.

💬 maflcko commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767054547)
Not sure about this. `tfm::format` is the only remaining function preventing the removal of `./test/lint/lint-format-strings.py`. I wanted to do this in a follow-up, but if this function is renamed, it will be harder, because all call sites will have to be renamed.

My preference would be to leave the name as-is and instead just delete the unchecked one, or rename that one to `format_maybe_throw` (or similar).

If you don't mind, it would be good to delete `./test/lint/lint-format-strings.py
...

💬 stickies-v commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767054801)
I don't agree it's redundant:
- it's not obvious from the tests that we're not just checking error messages, but that we're ensuring no errors are thrown, which is the difference with `tfm::format()`
- additional tests to try_format may be added in the future that test other aspects, so this acts as a little delineator

💬 mzumsande commented on pull request "AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress":
(https://github.com/bitcoin/bitcoin/pull/30909#issuecomment-2361367099)
Though one could also make a case for not allowing importdescriptor/rescan during background sync, with the reasoning "just wait a little bit longer" that doesn't really apply to the pruning case. Would be interesting to know if this has ever been discussed for assumeutxo.

🚀 fanquake merged a pull request: "Follow-up after AutoFile position caching: remove unused code"
(https://github.com/bitcoin/bitcoin/pull/30927)

💬 theStack commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2361384689)
Thanks for the reviews! Added a commit renaming the file build.h to bitcoin-build-info.h, to account for both the namespacing suggestions (https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2310630029) and the request for a more descriptive name (https://github.com/bitcoin/bitcoin/pull/30856#discussion_r1764891407). Let me know if another name is preferred (dashes vs. underscores? put "git" into filename or not?) and if a scripted-diff for moving bitcoin-config.h is preferred in thi
...

💬 theStack commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#discussion_r1767071727)
> Now that HAVE_BUILD_INFO is removed, this can be placed under the "normal" includes, no?

I left it where it is for now, as I thought it make sense if the comments and the preprocessor commands evaluating `BUILD_GIT_TAG` are immediately below, but no hard feelings. Happy to move only the include up if that is preferred.

👍 maflcko approved a pull request: "build: drop obj/ subdirectory for generated build.h"
(https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2315960896)
lgtm

Maybe squash, to avoid changing the file name twice?

💬 theStack commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2361420302)
> Maybe squash, to avoid changing the file name twice?

Makes sense yeah, done.