Bitcoin Core Github
44 subscribers
119K links
Download Telegram
pinheadmz closed an issue: "Transfer issues on myProject I need some ideas"
(https://github.com/bitcoin/bitcoin/issues/33400)
🤔 l0rinc reviewed a pull request: "coins: warn on oversized `-dbcache`"
(https://github.com/bitcoin/bitcoin/pull/33333#pullrequestreview-3226673018)
Thanks for the reviews, keep them coming :)
💬 l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2350374604)
Done
💬 l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2350374637)
Changed to `Assume`, but I don't find comments useful here.
💬 l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2350394510)
I don't like burdening the compiler this - and it makes errors and debugging uniform if we do it in tests - but I did make it `constexpr`, thanks for the hint
💬 l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2350374491)
Ok, I have applied something similar and updated the godbolt reproducer, thanks for the feedback
🤔 furszy reviewed a pull request: "net: do not apply whitelist permissions to onion inbounds"
(https://github.com/bitcoin/bitcoin/pull/33395#pullrequestreview-3226709131)
Code review ACK e46a7a547371317a4d116b9b1a314917508ea480
🤔 furszy reviewed a pull request: "key: use static context for libsecp256k1 calls where applicable"
(https://github.com/bitcoin/bitcoin/pull/33399#pullrequestreview-3226718930)
Concept ACK
💬 furszy commented on pull request "util/strencodings: guard SAFE_CHARS index and pre-reserve result":
(https://github.com/bitcoin/bitcoin/pull/33396#discussion_r2350414646)
what about changing the `SafeChars` enum to be an enum class instead? (enums auto-increment their value so you would just need to cast it to int here)
💬 achow101 commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#issuecomment-3294473221)
I think it would be better to use an `InitWarning` rather than just logging since people are unlikely notice
🤔 l0rinc requested changes to a pull request: "util/strencodings: guard SAFE_CHARS index and pre-reserve result"
(https://github.com/bitcoin/bitcoin/pull/33396#pullrequestreview-3226721389)
Not sure if this is an LLM generated patch or not, it's clear from the usages that the `int rule` doesn't need bounds check - but the reserve does make sense and we can swap the int param to the enum which could make sense.
💬 l0rinc commented on pull request "util/strencodings: guard SAFE_CHARS index and pre-reserve result":
(https://github.com/bitcoin/bitcoin/pull/33396#discussion_r2350425653)
There's no point in extracting this, the compilers are already smart enough to optimize this, see: https://godbolt.org/z/9rej918W7 (compare the SanitizeString_Direct and SanitizeString_Reference assemblies, they're the same).

But the string reserve that still make sense and we can change the parameter to the enum instead, e.g.
```patch
diff --git a/src/util/strencodings.cpp b/src/util/strencodings.cpp
index 289f49d46a..d1b99be943 100644
--- a/src/util/strencodings.cpp
+++ b/src/util/strencoding
...
💬 l0rinc commented on pull request "util/strencodings: guard SAFE_CHARS index and pre-reserve result":
(https://github.com/bitcoin/bitcoin/pull/33396#discussion_r2350409012)
the `rule` is an enum in reality, what's the reason for this addition?
💬 achow101 commented on pull request "test: Prevent disk space warning during node_init_tests":
(https://github.com/bitcoin/bitcoin/pull/33391#issuecomment-3294493195)
ACK bdf01c6f61262cd6e211ead3c0dbc66ccb48b32f
🚀 achow101 merged a pull request: "test: Prevent disk space warning during node_init_tests"
(https://github.com/bitcoin/bitcoin/pull/33391)
🤔 l0rinc reviewed a pull request: "key: use static context for libsecp256k1 calls where applicable"
(https://github.com/bitcoin/bitcoin/pull/33399#pullrequestreview-3226753026)
Thanks for fixing these.
Is there a way to separate these two usages by type somehow - I don't claim to fully understand when the state is needed and when a static can be used.
And would it make sense to rename the old `secp256k1_context_sign` references while we're here?

nit: typo in description `for the context paramter`
💬 l0rinc commented on pull request "key: use static context for libsecp256k1 calls where applicable":
(https://github.com/bitcoin/bitcoin/pull/33399#discussion_r2350433601)
if we still need this, we can move it to the scope where it's used (with the secp256k1_context_destroy call)
💬 l0rinc commented on pull request "key: use static context for libsecp256k1 calls where applicable":
(https://github.com/bitcoin/bitcoin/pull/33399#discussion_r2350455433)
Not my area of expertise, but is `secp256k1_context_sign` even the correct name here, wasn't that deprecated in favor of [`SECP256K1_CONTEXT_NONE`](https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1.h#L218)?
💬 l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#issuecomment-3294609537)
[Changed](https://github.com/bitcoin/bitcoin/compare/ec92151b538473792846db7d5bcabe4d6ae83c82..13fa0d0a433722f294854001b0561c079db12dbc) it to `InitWarning`, so now the warnings look like:
```
2025-09-16T02:17:00Z [warning] A 60000 MiB dbcache may be too large for a system memory of only 65536 MiB.
Warning: A 60000 MiB dbcache may be too large for a system memory of only 65536 MiB.
```
Thanks for the hint!
💬 adamandrews1 commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#issuecomment-3294736165)
@achow101 I've rebased this and updated the comment. No other changes.