💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1795269088)
To extend a bit: Previously (and after my pull right now), it was easy to toggle between translation and non-translation by replacing `_` with `Untranslated` (or vice-versa):

```diff
-strprintf(Untranslated("Cannot create database file %s"), filename);
+strprintf( _("Cannot create database file %s"), filename);
```

With your change, the function call order has to be changed as well.

💬 maflcko commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2404888004)
I liked the summary stats per folder produced by the current approach, but if others like other stuff better, I don't mind.

💬 fanquake commented on pull request "build: scripted-diff: drop config/ subdir for bitcoin-config.h":
(https://github.com/bitcoin/bitcoin/pull/30937#issuecomment-2404945576)
Guix Build:
```bash
9cf65a99fa046a1a3acbee14d6dec658b9fe7f4e55a4b6f50d063ebb4029e3eb guix-build-882f736d0a60/output/aarch64-linux-gnu/SHA256SUMS.part
46780332298b988bc9b4c2903d077f49978dc92250cf5221557ce4459383b18e guix-build-882f736d0a60/output/aarch64-linux-gnu/bitcoin-882f736d0a60-aarch64-linux-gnu-debug.tar.gz
72362b6f0394f4b8b1e69e4520c892d5b3e9f3d86788c8f0ee59f2b0d9e51717 guix-build-882f736d0a60/output/aarch64-linux-gnu/bitcoin-882f736d0a60-aarch64-linux-gnu.tar.gz
c4b5294efed908af
...

👍 fanquake approved a pull request: "build: scripted-diff: drop config/ subdir for bitcoin-config.h"
(https://github.com/bitcoin/bitcoin/pull/30937#pullrequestreview-2360160618)
ACK 882f736d0a607976ee5e3a6cbcb5385524bc72c6

🚀 fanquake merged a pull request: "build: scripted-diff: drop config/ subdir for bitcoin-config.h"
(https://github.com/bitcoin/bitcoin/pull/30937)

💬 dergoegge commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2405012504)
> I liked the summary stats per folder produced by the current approach, but if others like other stuff better, I don't mind.

The LLVM approach also supports that:


💬 ismaelsadeeq commented on issue "Fee Estimation via Fee rate Forecasters tracking issue":
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2405065185)
This issue was opened to track the effort to improve the Bitcoin core fee estimator. This comment will provide information on the status of this project and some key ideas on how to achieve this.

**Completed Tasks so far:**
- **Enabled asynchronous block policy fee estimator updates**: By making it a client of the validation interface.

- **Empirical data analysis**: To determine which forecasting strategy provides the most accurate estimates while withstanding common attack vectors.
- De
...

💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2405071446)
> Can you provide more details here? IIUC a hash will still be calculated, just not compared, so this may only drop one hash call, with a possible maximum speed-up of 2x. However, it would be good to actually measure the difference. Otherwise, I am not sure if this is worth it.

This is not about performance. It makes the `p2p_transport_serialization` fuzz target simpler because we can remove the code that assists the checksum and magic bytes creation. So, we can achieve the same with less LoC
...

💬 maflcko commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2405082968)
Ok, then I am nack-ish on this. Generally, test code should follow real code, not the other way round, unless there is a reason. Saving 15 lines of test-only code seems a weak reason to me, but I don't mind if others like this.

💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2405088673)
Since everyone agrees on removing Honggfuzz netdriver, I think we can change the approach to simply just remove it from the documentation.

💬 jonatack commented on issue "Fee Estimation via Fee rate Forecasters tracking issue":
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2405100792)
> Create estimatefee RPC

Four years ago I was beginning to implement the roadmap as described in https://github.com/bitcoin/bitcoin/pull/20484#issuecomment-734786305, beginning with a `setfeerate` RPC in https://github.com/bitcoin/bitcoin/pull/20391 and a planned `estimatefeerate` RPC, both using sat/vB units. Was considering picking these up again. Presumably orthogonal, apart from coordinating on the RPC naming.

🤔 TheCharlatan reviewed a pull request: "log: Enforce trailing newline"
(https://github.com/bitcoin/bitcoin/pull/30929#pullrequestreview-2360363711)
Post-merge ACK

I think this really showed the limits of having our own clang-tidy checks. They don't lend themselves well to code that is evolving, may receive renames, or new methods.

💬 jonatack commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1795456841)
Per feedback in https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1759115295, begin this sentence with "Assuming the build directory is named `build`, running..."

I agreed with the feedback not to add it where it was already mentioned in other files, but in this file there is no other mention.

🤔 jonatack reviewed a pull request: "doc: cmake: prepend "build" to functional/test_runner.py"
(https://github.com/bitcoin/bitcoin/pull/30859#pullrequestreview-2360372434)
LGTM modulo comment

👍 TheCharlatan approved a pull request: "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment"
(https://github.com/bitcoin/bitcoin/pull/30666#pullrequestreview-2360426595)
Re-ACK 0bd53d913c1c2ffd2d0779f01bc51c81537b6992

💬 maflcko commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#discussion_r1795480094)
It seems `CLIENT_NAME` is already used in a different (non-build) context to denote the "user agent name". Maybe a preparatory commit could be added to change that to `UA_NAME`, for clarity and to avoid a name clash?


```
$ git grep --line-number CLIENT_NAME
src/clientversion.cpp:23:const std::string CLIENT_NAME("Satoshi");
src/clientversion.h:36:extern const std::string CLIENT_NAME;
src/init.cpp:1470: strSubVersion = FormatSubVersion(CLIENT_NAME, CLIENT_VERSION, uacomments);

💬 TheCharlatan commented on pull request "build: scripted-diff: drop config/ subdir for bitcoin-config.h":
(https://github.com/bitcoin/bitcoin/pull/30937#issuecomment-2405170985)
Post-merge ACK 882f736d0a60

Guix build (aarch64):
```
9cf65a99fa046a1a3acbee14d6dec658b9fe7f4e55a4b6f50d063ebb4029e3eb guix-build-882f736d0a60/output/aarch64-linux-gnu/SHA256SUMS.part
46780332298b988bc9b4c2903d077f49978dc92250cf5221557ce4459383b18e guix-build-882f736d0a60/output/aarch64-linux-gnu/bitcoin-882f736d0a60-aarch64-linux-gnu-debug.tar.gz
72362b6f0394f4b8b1e69e4520c892d5b3e9f3d86788c8f0ee59f2b0d9e51717 guix-build-882f736d0a60/output/aarch64-linux-gnu/bitcoin-882f736d0a60-aarch
...

💬 ismaelsadeeq commented on issue "Fee Estimation via Fee rate Forecasters tracking issue":
(https://github.com/bitcoin/bitcoin/issues/30392#issuecomment-2405172852)
> Both using sat/vB units.

This is a good idea.

> Beginning with a setfeerate RPC in https://github.com/bitcoin/bitcoin/pull/20391.

I learned about this issue after @murchandamus's review comment https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1526664705. I think we should just unify `settxfee` and your `setfeerate` into one RPC?

> Was considering picking these up again. Presumably orthogonal, apart from coordinating on the RPC naming.

Happy to review when you do.
...

💬 fanquake commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#discussion_r1795498723)
I think if we end up doingthat, we should extend this to renaming other source variables that exist in the PACKAGE_ namespace. This PR as-is is already changing things such that naming becomes more inconsistent in that regard.

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1795560402)
this is definitely problematic, but I can't seem to figure out a way to hit it. In prod this would "just" cause it to not call `m_txrequest.ForgetTxHash(ptx->GetWitnessHash());` for it, but would have to be in a 1p1c package setting.

To retain current behavior, I think it can just be expanded to
```suggestion
// DIFFERENT_WITNESS has "valid" status, but we still want to stop requesting it
if (!Assume(state.IsInvalid() ||
tx_result.m_result_type == MempoolAcceptResult::Res
...