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

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1664074217)
> Why make mappings for all local IPv6 addresses?

It's the sensible choice IMO. Some routers will assign a whole bunch of routable IPv6 addresses but only allow pinholing for some. It's doesn't hurt to try.

> Also, it just occurred to me, GetLocalAddresses() operates regardless of -bind= option

Yes. But mind that `-natpmp` and `-pnp` are set to 0 if `-bind` is specified. In that case we assume the user has a more complex configuration and will just do their own mapping.

✅ maflcko closed an issue: "Memory leak loading legacy wallet (BDB 4.8.30)"
(https://github.com/bitcoin/bitcoin/issues/27283)

💬 maflcko commented on issue "Memory leak loading legacy wallet (BDB 4.8.30)":
(https://github.com/bitcoin/bitcoin/issues/27283#issuecomment-2205913297)
Many workarounds have been mentioned in the discussion above. For example:

* disable privdb temporarily
* upgrade bdb
* suppress the warning
* remove bdb

Closing for now. I am happy to answer questions how to implement the workarounds, but I don't think there is anything that can be done in this repo that hasn't already an open pull request. If a pull request is missing, please create one, or explain exactly what should be implemented.