💬 1440000bytes commented on pull request "optimization: increase default LevelDB write batch size to 64 MiB":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2587807448)
If there are tradeoffs (speed, memory usage etc.) involved in changing default batch size, then it could remain the same.
Maybe a config option can be provided to change it.
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2587807448)
If there are tradeoffs (speed, memory usage etc.) involved in changing default batch size, then it could remain the same.
Maybe a config option can be provided to change it.
💬 sipa commented on pull request "optimization: increase default LevelDB write batch size to 64 MiB":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2587815370)
There is a config option. This is about changing the dedault.
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2587815370)
There is a config option. This is about changing the dedault.
💬 marcofleon commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2587839584)
Updated the `-checkpoints` options and added a warning (similar to `-upnp`). Also removed `blockheader_testnet3.hex`. Thanks!
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2587839584)
Updated the `-checkpoints` options and added a warning (similar to `-upnp`). Also removed `blockheader_testnet3.hex`. Thanks!
💬 1440000bytes commented on pull request "optimization: increase default LevelDB write batch size to 64 MiB":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2587860455)
> There is a config option. This is about changing the dedault.
Just realized [`dbbatchsize`](https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/src/init.cpp#L489) already exists.
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2587860455)
> There is a config option. This is about changing the dedault.
Just realized [`dbbatchsize`](https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/src/init.cpp#L489) already exists.
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2587932545)
I just realized the coinbase transactions are unspendable due to the `unexpected-witness` and hardcode SegWit activation height on mainnet.
It doesn't matter for this PR, but I'm working on a new edition that uses legacy addresses and also checks that the coinbase is spendable. Will push here or, if it's already merged, to a followup.
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2587932545)
I just realized the coinbase transactions are unspendable due to the `unexpected-witness` and hardcode SegWit activation height on mainnet.
It doesn't matter for this PR, but I'm working on a new edition that uses legacy addresses and also checks that the coinbase is spendable. Will push here or, if it's already merged, to a followup.
💬 pinheadmz commented on issue "Remove libevent as a dependency (HTTP / cli / torcontrol)":
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2587939419)
@pablomartin4btc thanks, I added this test to my branch:
```cpp
// Multiple parameters, some characters encoded
uri = "/rest/endpoint/someresource.json?p1=v1%20&p2=100%25";
BOOST_CHECK_EQUAL(GetQueryParameterFromUri(uri.c_str(), "p1").value(), "v1 ");
BOOST_CHECK_EQUAL(GetQueryParameterFromUri(uri.c_str(), "p2").value(), "100%");
```
You're totally right, libevent decodes `%XX` but only after it has parsed the URI. Meaning the special characters `?` `&` and `=` (prob
...
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2587939419)
@pablomartin4btc thanks, I added this test to my branch:
```cpp
// Multiple parameters, some characters encoded
uri = "/rest/endpoint/someresource.json?p1=v1%20&p2=100%25";
BOOST_CHECK_EQUAL(GetQueryParameterFromUri(uri.c_str(), "p1").value(), "v1 ");
BOOST_CHECK_EQUAL(GetQueryParameterFromUri(uri.c_str(), "p2").value(), "100%");
```
You're totally right, libevent decodes `%XX` but only after it has parsed the URI. Meaning the special characters `?` `&` and `=` (prob
...
💬 l0rinc commented on pull request "optimization: increase default LevelDB write batch size to 64 MiB":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2587954338)
> If that spike exceeds the memory the process can allocate it causes a crash
Thanks for the context, @sipa.
On the positive side, the extra allocation is constant (or at least non-proportional with the usage) and it's narrowing the window for other crashes during flushing (https://github.com/bitcoin/bitcoin/pull/30611 will also likely help here).
This change may also enable another one (that I'm currently re-measuring to be sure), which seems to halve the remaining flush time again (by sor
...
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2587954338)
> If that spike exceeds the memory the process can allocate it causes a crash
Thanks for the context, @sipa.
On the positive side, the extra allocation is constant (or at least non-proportional with the usage) and it's narrowing the window for other crashes during flushing (https://github.com/bitcoin/bitcoin/pull/30611 will also likely help here).
This change may also enable another one (that I'm currently re-measuring to be sure), which seems to halve the remaining flush time again (by sor
...
💬 maflcko commented on pull request "ci: Bump centos stream 10":
(https://github.com/bitcoin/bitcoin/pull/31593#issuecomment-2588022144)
Actually bz2 isn't needed anymore either after https://github.com/bitcoin/bitcoin/pull/29895
(https://github.com/bitcoin/bitcoin/pull/31593#issuecomment-2588022144)
Actually bz2 isn't needed anymore either after https://github.com/bitcoin/bitcoin/pull/29895
👍 darosior approved a pull request: "test: Add mockable steady clock, tests for PCP and NATPMP implementations"
(https://github.com/bitcoin/bitcoin/pull/31022#pullrequestreview-2529393336)
ACK a50e1b5af747134b3e102ef7867ee262ba410700
`PROTOCOL_ERROR` is the only variant of `MappingError` which is not covered by the unit test. If you feel that's worth including it i wrote https://github.com/darosior/bitcoin/commit/be7482c2f6f754cfa4c205cff66ba9c689d426f5 to cover the 3 code paths in which it occurs.
(https://github.com/bitcoin/bitcoin/pull/31022#pullrequestreview-2529393336)
ACK a50e1b5af747134b3e102ef7867ee262ba410700
`PROTOCOL_ERROR` is the only variant of `MappingError` which is not covered by the unit test. If you feel that's worth including it i wrote https://github.com/darosior/bitcoin/commit/be7482c2f6f754cfa4c205cff66ba9c689d426f5 to cover the 3 code paths in which it occurs.
💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1901969607)
This does not need to be optional.
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1901969607)
This does not need to be optional.
💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913713048)
Ah, maybe it does for #31014.
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913713048)
Ah, maybe it does for #31014.
💬 darosior commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2588044409)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2588044409)
Concept ACK.
💬 darosior commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913751718)
Seems worth keeping, at least for a few versions?
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913751718)
Seems worth keeping, at least for a few versions?
🤔 l0rinc requested changes to a pull request: "consensus: Remove checkpoints (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2547740151)
Concept ACK
The remaining "checkpoint" references seem unrelated.
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2547740151)
Concept ACK
The remaining "checkpoint" references seem unrelated.
💬 l0rinc commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913752692)
usually there's a single line per arg here
```suggestion
argsman.AddArg("-checkpoints", "", ArgsManager::ALLOW_ANY, OptionsCategory::HIDDEN); // Checkpoints were removed. We keep `-checkpoints` as a hidden arg to display a more user friendly error when set.
```
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913752692)
usually there's a single line per arg here
```suggestion
argsman.AddArg("-checkpoints", "", ArgsManager::ALLOW_ANY, OptionsCategory::HIDDEN); // Checkpoints were removed. We keep `-checkpoints` as a hidden arg to display a more user friendly error when set.
```
💬 l0rinc commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913746337)
you can likely remove `#include <ranges>` now
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913746337)
you can likely remove `#include <ranges>` now
💬 l0rinc commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913753340)
this is already obvious from the error, the comment is redundant
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913753340)
this is already obvious from the error, the comment is redundant
💬 l0rinc commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913741821)
To guard against `BLOCK_HEADER_LOW_WORK` changing its numeric value now (to 8 instead of previous 9), we could hard-code the previous value to the remaining ones
```C++
enum class BlockValidationResult {
BLOCK_RESULT_UNSET = 0, //!< initial value. Block has not yet been rejected
BLOCK_CONSENSUS = 1, //!< invalid by consensus rules (excluding any below reasons)
BLOCK_CACHED_INVALID = 2, //!< this block was cached as being invalid and we didn't store the reason why
...
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913741821)
To guard against `BLOCK_HEADER_LOW_WORK` changing its numeric value now (to 8 instead of previous 9), we could hard-code the previous value to the remaining ones
```C++
enum class BlockValidationResult {
BLOCK_RESULT_UNSET = 0, //!< initial value. Block has not yet been rejected
BLOCK_CONSENSUS = 1, //!< invalid by consensus rules (excluding any below reasons)
BLOCK_CACHED_INVALID = 2, //!< this block was cached as being invalid and we didn't store the reason why
...
💬 l0rinc commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913754168)
`#include <map>` and a few others can likely be removed now
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913754168)
`#include <map>` and a few others can likely be removed now
💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913774345)
Yes there is one edge-case there where it is necessary for it to be optional, unfortunately, and no length is directlly available.
i guess we could also make it non-optional then do something else at the call-site there (e.g. create a `AddrLenFromFamily` function or such), fine with me too.
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913774345)
Yes there is one edge-case there where it is necessary for it to be optional, unfortunately, and no length is directlly available.
i guess we could also make it non-optional then do something else at the call-site there (e.g. create a `AddrLenFromFamily` function or such), fine with me too.