💬 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.

💬 maflcko commented on issue "I want to set up a node quickly, do you have a snapshot？":
(https://github.com/bitcoin/bitcoin/issues/30382#issuecomment-2205895693)
> There are some **unofficial, unrelated to this project** assumeutxo snapshots

assumeutxo is a test-only feature for now, not enabled on mainnet, so I don't think this can help here at this point in time. I haven't checked what the site offers, but downloading a datadir from an untrusted source is dangerous and can lead to loss-of-funds, or any other type of bugs.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1664067870)
Yes, that makes sense, in the case of onlynet=ipv6 we wouldn't want to advertise a port on ipv4 and vice versa.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1664071010)
i've thought about this, but due to how bitcoin handles non-standard ports, we really want the configured external port, not another one, i think it makes sense to keep trying to get it, even if the router assigns us a different one once.