💬 willcl-ark commented on issue "Memory leak loading legacy wallet (BDB 4.8.30)":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-2205804346)
Can we close this and https://github.com/bitcoin/bitcoin/issues/19034 as "unplanned"?

In theory v28.0 will (hopefully) be the final version we will read BDB wallets at all, and we now have our own [BDB parser](https://github.com/bitcoin/bitcoin/pull/26606), and will (soon) be able to [migrate without BDB](https://github.com/bitcoin/bitcoin/pull/26596).

I don't see these issues ever being fixed, and therefore they are not useful to keep open IMO...

💬 ryanofsky commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1664002278)
In commit "refactor: PSBTError::MISSING_INPUTS" (a05f4ca241045e8a4b295760590805f961c2e5e7)

I wonder if "invalid transaction inputs" might be a clearer message for a user than "invalid inputs." (I had suggested "invalid inputs" but in this context it seems vague.)

👍 ryanofsky approved a pull request: "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN"
(https://github.com/bitcoin/bitcoin/pull/30212#pullrequestreview-2156118365)
Agree on not calling the commits which change error messages refactoring. I do like first commit making error constant more consistent with error message, but agree it is not essential.

💬 ryanofsky commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1663994169)
re: https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1663945296

Good catch. It is inconsistent to have added backwards compatible aliases previously when renaming and not add them now when renaming again. But it looks like the backwards compatibility section was added in 2014 when it was might have less clear whether aliases were needed. I'd say now aliases are probably not needed and it would be better to remove this section.

📝 maflcko opened a pull request: "util: Catch translation string errors at compile time"
(https://github.com/bitcoin/bitcoin/pull/30383)
The translation helper function `_()` has many problems. For example, the following compiles:

```cpp
auto ptr{"wrong"};
_(ptr);
_(nullptr);
_(0);
_(NULL);
```

However, it is wrong, because none of the arguments passed to the function can be picked up by the translation tooling for transifex.

Fix all issues by enforcing only real string literals can be passed to the function.

💬 maflcko commented on pull request "util: Catch translation string errors at compile time":
(https://github.com/bitcoin/bitcoin/pull/30383#issuecomment-2205818693)
Can be tested by looking at the CI failure, which should pop up soon, or by manually inserting the C++ code from the pull request description.

💬 maflcko commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1664014943)
> I guess it could make sense to mark `_(const char*)` as `consteval` to catch those issues at compile time?

Done in https://github.com/bitcoin/bitcoin/pull/30383

🤔 glozow reviewed a pull request: "chainparams: Change nChainTx type to uint64_t"
(https://github.com/bitcoin/bitcoin/pull/29656#pullrequestreview-2156159675)
concept ACK

💬 glozow commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#discussion_r1664016248)
64d35be20c43f764e5c22657085a790d0adcfe22 should remove this todo comment

💬 fanquake commented on pull request "util: Catch translation string errors at compile time":
(https://github.com/bitcoin/bitcoin/pull/30383#issuecomment-2205828992)
```bash
CXX test/fuzz/fuzz-string.o
CXX libbitcoin_node_a-validation.o
test/fuzz/string.cpp:104:13: error: call to consteval function 'ConstevalStringLiteral::ConstevalStringLiteral' is not a constant expression
(void)_(random_string_1.c_str());
1 error generated.

validation.cpp:5750:30: error: call to consteval function 'ConstevalStringLiteral::ConstevalStringLiteral' is not a constant expression
return util::Error{_(reason)};
^

...

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1664020423)
i don't think that's the case in practice? but even so, doing this differently would really complicate things too much, changes in network configuration would be noticed the next update interval anyway?

💬 ryanofsky commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1664025326)
re: https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663631973

> It is not possible to pass a raw c-string pointer to transifex for translation. Only string literals can be translated.

In case it's not clear, the fix for this problem and the previous [unnecessary translation](https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1663611194) problem is probably to replace `_()` with `Untranslated()` everywhere where in this PR when converting strings to bilingual_str. If we ad
...

💬 hebasto commented on pull request "util: Catch translation string errors at compile time":
(https://github.com/bitcoin/bitcoin/pull/30383#issuecomment-2205838536)
Concept ACK on the PR's goal.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1664026678)
No, this doesn't implement the exact retry logic, it seems excessively complicated for communicating with a local router, and re-using the same code for NAT-PMP and PCP keeps the code straighforward.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1664031826)
This is intentional. For IPv4 we provide the all-zeroes address, because the router will use its own (NAT), for IPv6 we *want* the external address to be the same as the local address (NAT is not a thing, we want pinholing).

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1664034915)
As we receive the UDP packet at all, i think that should be correct. But sure...

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1664036768)
Again, i intentionally use a simple imlementation here. i don't want to complicate the code too much. As i understand, this covers the practical cases. It chooses the minimum because we don't want holes in coverage while renewing the mapping more than necessary doesn't hurt.

🤔 glozow reviewed a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2156187877)
ACK c1d6e525131cb9e54bbedb79ea64e2469a77aed8

💬 glozow commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1664034005)
Question tangential to this PR: This seems to mean the script execution cache needs something to synchronize access, and it is currently implicitly using `cs_main`? I don't see any threads other than message handler that access it though.

💬 glozow commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1664039214)
c1d6e52513

nit: could do a `static_assert(DEFAULT_VALIDATION_CACHE_BYTES == DEFAULT_SIGNATURE_CACHE_BYTES + DEFAULT_SCRIPT_EXECUTION_CACHE_BYTES)`. Should still hold if allocation of `-maxsigcachesize` changes from 50/50.