💬 mcelrath commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2361214383)
Working on a stale block is exactly the problem here, because of inconsistent responses. I think we have a miscommunication here... It sound like you're saying there's no way to avoid working on a stale block...

💬 mcelrath commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2361224527)
@sipa we're planning on switching to the new IPC mechanism anyway, that StratumV2 is also using. Do you have any comments about synchronicity in that context?

FWIW I think I'm the only person ever to have used ZMQ. My `contrib/zmq/zmq_sub.py` file no longer works due to python async changes. I'm also planning on submitting a PR to update it. But maybe I agree ZMQ might be a mistake. OTOH I think polling in the 21st century is also a mistake.

💬 stickies-v commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1766980655)
Addressed in #30928.

You're right that the unconditional logging should make these errors quite likely to be caught in review, and that there is no consistency in when we catch formatting exceptions or not. In #30928 I tried to address the latter by enforcing run-time exceptions are handled whenever we use `util::ConstevalFormatString`, which should correlate with needing to prioritize run-time robustness.

💬 l0rinc commented on pull request "build: speed up by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2361230687)
Now that https://github.com/bitcoin/bitcoin/pull/30888 is merged, the difference is not that dramatic: I'm measuring a `0.5%` change:

### latest `master` 2x
`% ccache --clear && git clean -fxd && cmake -B build && time cmake --build build -j10`
> cmake --build build -j10 994.42s user 91.78s system 832% cpu 2:10.47 total
> cmake --build build -j10 993.43s user 91.40s system 832% cpu 2:10.30 total

### rebased branch 2x
`% ccache --clear && git clean -fxd && cmake -B build && time cmake
...

📝 maflcko opened a pull request: " log: Enforce trailing newline at compile time "
(https://github.com/bitcoin/bitcoin/pull/30929)
There are many problems around missing a trailing newline while logging:
* Leaving a line unterminated is racy and can cause content corruption by mixing log lines from different sources.
* It requires extra code like `m_started_new_line` to keep track of, which is annoying and pointless to maintain, because it is currently dead code, see https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1684380835.
* It requires a standalone `unterminated-logprintf` clang-tidy plugin, which is unmain
...

💬 theStack commented on pull request "test: switch MiniWallet padding unit from weight to vsize":
(https://github.com/bitcoin/bitcoin/pull/30718#discussion_r1766995759)
> without this multiplier was there an issue in this PR?

Yes, without it the assertion failed (`AssertionError: 75000 <= 135680`).

💬 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