💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#issuecomment-3236024582)
Are there any stats on increased coverage, or is this just checking more values in the same paths?

lgtm ACK 84927d37745920412b270ad3502308f0ad8cb7ca 📠

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/
...

🤔 maflcko reviewed a pull request: "fuzz: enhance wallet_fees by mocking mempool stuff"
(https://github.com/bitcoin/bitcoin/pull/33210#pullrequestreview-3167583982)
.

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2309317896)
nit in 0fe7c25a7a6086cabdf96c6ebd0e0de9b6e4176d: just `CMutableTransaction mtx{};` is enough, no need to list the type twice.

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2309376831)
nit in https://github.com/bitcoin/bitcoin/commit/79b28c0740c57aac5b91fd3aff20bc46b9229376: Seems to pass right now, even though it runs into fs path errors. Seems fine for now, but in the future there could be a pure abstract base class below `CBlockPolicyEstimator` (used by wallet_fees) and `CBlockPolicyEstimator` as well as `FuzzedBlockPolicyEstimator` could derive from it (`final`). But this can be done in a follow-up, if there is need to.

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2309336307)
nit in https://github.com/bitcoin/bitcoin/commit/0fe7c25a7a6086cabdf96c6ebd0e0de9b6e4176d: Why only assert `>0` and not `>fee`, or even `==fee+min_inc_fee`?

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2309337870)
nit in https://github.com/bitcoin/bitcoin/commit/0fe7c25a7a6086cabdf96c6ebd0e0de9b6e4176d: Can just be a default-constructed CScript?

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2309371283)
also clang-format for new code?

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2309370885)
nit in 79b28c0740c57aac5b91fd3aff20bc46b9229376: Could drop the `C` for new code? `FuzzedBlockPolicyEstimator`.

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2309394350)
or maybe just move `MockMempoolMinFee` to the src/test/util/mempool utils and use it? (Skip if the target fee is less than inc relay fee or min relay fee)?

💬 jsarenik commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#issuecomment-3236117393)
Tested ACK 7d1c3ce7f0f0

Thanks!

💬 maflcko commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2309527992)
7b9a5d53c8ae47d2789e31306d035843d870f9a8: This happens to work, but generally it is not fine to replace `c_str()` with `string_view::data()`, because it is lacking the null-terminator. To restore the null-terminator in all cases, you'll have to create a copy again:

```cpp
std::string{node_arg}.c_str()

💬 cedwies commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2309613567)
Curious to see what you find. I’ll try to follow if you open an issue/PR on it.

💬 maflcko commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2309644417)

configuration -> configurations [“across all binding configuration” should be pluralized to “configurations” for grammatical correctness]

💬 ajtowns commented on pull request "doc: unify `datacarriersize` warning with release notes":
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3236344537)
> Ultimately the reason transifex thinks 121 additional strings need to be translated is because the `.xlf` files contain an id for each string which is based upon the position of a string in the `bitcoin_en.ts` file which is automatically generated from the source code using `lupdate`

Is this context info useful? Line numbers will change whenever some previous function in the file gets a non-trivial edit... Would it be better to add `-locations none` to the LCONVERT invocation in translate.c
...

📝 l0rinc opened a pull request: "translations: recreate baseline to simplify testing"
(https://github.com/bitcoin/bitcoin/pull/33270)
### Summary

Previously, recalculating the translation string templates caused a misalignment: after any changed string, subsequent entries were re-numbered and old values received new “stable” IDs.

You can reproduce by checking out https://github.com/bitcoin/bitcoin/pull/33224 and running:

```bash
cmake --preset dev-mode -DWITH_USDT=OFF -DENABLE_IPC=OFF && cmake --build build_dev_mode --target translate
```
This will change many translation IDs:
```bash
git diff | egrep '^[+-].+tra
...

💬 l0rinc commented on pull request "doc: unify `datacarriersize` warning with release notes":
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3236378613)
I have added a tool which could be useful for making sure edits like this aren't problematic anymore - feedback is welcome on overall direction: https://github.com/bitcoin/bitcoin/pull/33270

💬 hodlinator commented on pull request "clang-format: align brace-after-struct and *-class formatting":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2309691268)
The line-counts where from the resulting `git diff`s. Admittedly they are imprecise due to surrounding context and other clang-format changes, but they give a rough relative estimate.

Thanks for vibing, almost double on same line. I'd say that does matter.

👍 maflcko approved a pull request: "rpc: refactor: use string_view in Arg/MaybeArg"
(https://github.com/bitcoin/bitcoin/pull/32983#pullrequestreview-3167873647)
Looks like a nice change using more named `Arg` and `MaybeArg` helpers, removing the manual `params[idx].isNull` and `params[idx].get_str` legacy handling.

Not a blocker, but it would be nice to fix the theoretical c_str/data issue.

lgtm ACK 24d48638795ba61e8a420df4b831281455e208a7 🏉

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noA
...

💬 maflcko commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2309542222)
7b9a5d53c8ae47d2789e31306d035843d870f9a8: update docstring?

💬 maflcko commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2309677395)
```suggestion
std::string subnet_arg{help.Arg<std::string_view>("subnet"};
```

nit in the last commit: Use the named `Arg` helper for new code?