💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1913418467)
88b67c95cbc9c3ae75ccbc39e4aba215b9910b8b nit: `wallet =` would be more consistent
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1913418467)
88b67c95cbc9c3ae75ccbc39e4aba215b9910b8b nit: `wallet =` would be more consistent
💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1913428791)
9452f06a0283299c420a52f6b7f59f484cb60ca3 nit: maybe call it `funding_wallet =`, or `default =` for consistency with `test_basic`. Alternatively you could introduce a helper `self.fund(address, amount)`
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1913428791)
9452f06a0283299c420a52f6b7f59f484cb60ca3 nit: maybe call it `funding_wallet =`, or `default =` for consistency with `test_basic`. Alternatively you could introduce a helper `self.fund(address, amount)`
💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1913528143)
9452f06a0283299c420a52f6b7f59f484cb60ca3: maybe add a comment to explain what's going on here. In particular why `pkh_script` needs to be imported, even though the legacy wallet tracks a `pkh(pubkey)` for every address you generate.
IIUC:
- normally the legacy wallet doesn't consider the `wsh(pkh(pubkey))` variant
- It only has `pkh(pubkey)`, `wpkh(pubkey)` and `sh(wpkh(pukey)`
- but `pkh(pubkey)` is not in `mapScripts` (?)
- The two `importaddress` calls add `pkh(pubkey)` and `wsh(pkh
...
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1913528143)
9452f06a0283299c420a52f6b7f59f484cb60ca3: maybe add a comment to explain what's going on here. In particular why `pkh_script` needs to be imported, even though the legacy wallet tracks a `pkh(pubkey)` for every address you generate.
IIUC:
- normally the legacy wallet doesn't consider the `wsh(pkh(pubkey))` variant
- It only has `pkh(pubkey)`, `wpkh(pubkey)` and `sh(wpkh(pukey)`
- but `pkh(pubkey)` is not in `mapScripts` (?)
- The two `importaddress` calls add `pkh(pubkey)` and `wsh(pkh
...
💬 brunoerg commented on pull request "fuzz: Expand script verification flag testing to segwit v0 and tapscript":
(https://github.com/bitcoin/bitcoin/pull/31460#discussion_r1913547384)
In 70ec9f20341a5337372597ed481b048e40982b55: Instead of asserting that the program size is greater or equal to uint256 size, could we assert that the program size is exactly the `WITNESS_V0_SCRIPTHASH_SIZE`?
(https://github.com/bitcoin/bitcoin/pull/31460#discussion_r1913547384)
In 70ec9f20341a5337372597ed481b048e40982b55: Instead of asserting that the program size is greater or equal to uint256 size, could we assert that the program size is exactly the `WITNESS_V0_SCRIPTHASH_SIZE`?
👍 ryanofsky approved a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2547308805)
Code review ACK b776e40554c8a315d832f3996d14ff2e3ae7b8cb. Just whitespace changes since last review
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2547308805)
Code review ACK b776e40554c8a315d832f3996d14ff2e3ae7b8cb. Just whitespace changes since last review
💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2587744121)
You can delete `blockheader_testnet3.hex` as it's no longer used and testnet3 may get deprecated anyway. #31583 introduces something similar based on mainnet.
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2587744121)
You can delete `blockheader_testnet3.hex` as it's no longer used and testnet3 may get deprecated anyway. #31583 introduces something similar based on mainnet.
👍 rkrux approved a pull request: "doc: Archive 28.1 release notes"
(https://github.com/bitcoin/bitcoin/pull/31630#pullrequestreview-2547321379)
reACK bb5f76ee013ee439f70e86319d22f308db8a24ea
(https://github.com/bitcoin/bitcoin/pull/31630#pullrequestreview-2547321379)
reACK bb5f76ee013ee439f70e86319d22f308db8a24ea
💬 vasild commented on pull request "test: avoid internet traffic":
(https://github.com/bitcoin/bitcoin/pull/31646#issuecomment-2587747739)
`3eb4ad7740...66dbfaca22`: dedup the definition of the unreachable proxy arg variable, thanks @brunoerg
(https://github.com/bitcoin/bitcoin/pull/31646#issuecomment-2587747739)
`3eb4ad7740...66dbfaca22`: dedup the definition of the unreachable proxy arg variable, thanks @brunoerg
💬 rkrux commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1913561146)
Needs a blank line after line 9.
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1913561146)
Needs a blank line after line 9.
💬 rkrux commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1913559740)
This doesn't show up as a table when I preview the md file.
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1913559740)
This doesn't show up as a table when I preview the md file.
💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913589077)
> It seems wrong that calling `SaturatingLeftShift` on a negative input can wrap around to 0.
My understanding was that left shift `a << b` with negative `a` was UB in C++, which is why I thought saturating negative inputs to 0 was the most sensible noexcept approach. Thanks for pointing out (as per your cppreference link) that since C++20 this is indeed defined, which definitely means our implementation can (and should) too, nice!
> It also seems like a footgun that these left shift funct
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913589077)
> It seems wrong that calling `SaturatingLeftShift` on a negative input can wrap around to 0.
My understanding was that left shift `a << b` with negative `a` was UB in C++, which is why I thought saturating negative inputs to 0 was the most sensible noexcept approach. Thanks for pointing out (as per your cppreference link) that since C++20 this is indeed defined, which definitely means our implementation can (and should) too, nice!
> It also seems like a footgun that these left shift funct
...
💬 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.