💬 pinheadmz commented on issue "Faster way to get block with prevouts in JSON-RPC":
(https://github.com/bitcoin/bitcoin/issues/30495#issuecomment-2587573080)
> Support binding to unix domain sockets
This is a common request, but requires replacing libevent, which I'm working on: https://github.com/bitcoin/bitcoin/issues/31194
(https://github.com/bitcoin/bitcoin/issues/30495#issuecomment-2587573080)
> Support binding to unix domain sockets
This is a common request, but requires replacing libevent, which I'm working on: https://github.com/bitcoin/bitcoin/issues/31194
💬 sipa commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2587635968)
If we go through with this, perhaps it's worth keeping the `-checkpoints` option around, just to trigger a startup warning if provided (as opposed to an error if someone still has it in the config).
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2587635968)
If we go through with this, perhaps it's worth keeping the `-checkpoints` option around, just to trigger a startup warning if provided (as opposed to an error if someone still has it in the config).
💬 davidgumberg commented on pull request "ci: Allow build dir on CI host":
(https://github.com/bitcoin/bitcoin/pull/31428#issuecomment-2587639604)
lgtm ACK https://github.com/bitcoin/bitcoin/commit/8888ee4403fa62972c49e18752695d15fd32c0b0
(https://github.com/bitcoin/bitcoin/pull/31428#issuecomment-2587639604)
lgtm ACK https://github.com/bitcoin/bitcoin/commit/8888ee4403fa62972c49e18752695d15fd32c0b0
💬 achow101 commented on pull request "doc: Archive 28.1 release notes":
(https://github.com/bitcoin/bitcoin/pull/31630#issuecomment-2587656651)
> ACK, but probably want to fix [bitcoin-core/bitcoincore.org#1100 (comment)](https://github.com/bitcoin-core/bitcoincore.org/pull/1100#discussion_r1910510272) first.
Done
(https://github.com/bitcoin/bitcoin/pull/31630#issuecomment-2587656651)
> ACK, but probably want to fix [bitcoin-core/bitcoincore.org#1100 (comment)](https://github.com/bitcoin-core/bitcoincore.org/pull/1100#discussion_r1910510272) first.
Done
💬 petamas commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2587664804)
@hebasto: Did you get a reply in the Developer Community issue? I've run into the same ICE at my workplace, but I don't have access to the linked issue. ("We were unable to get this feedback item. It could be because you don't have access to it or it doesn't exist")
Thanks!
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2587664804)
@hebasto: Did you get a reply in the Developer Community issue? I've run into the same ICE at my workplace, but I don't have access to the linked issue. ("We were unable to get this feedback item. It could be because you don't have access to it or it doesn't exist")
Thanks!
💬 dergoegge commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2587672306)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2587672306)
Concept ACK
💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913521985)
In commit "util: Add integer left shift helpers" (e0a541702def3a4c6cce8e44b6ebe8d608edad4e)
It seems wrong that calling `SaturatingLeftShift` on a negative input can wrap around to 0. The point of saturating operations should be that they don't wrap around, but effectively perform operations with full precision and just clamp the result values. In c++ [left shift](https://en.cppreference.com/w/cpp/language/operator_arithmetic#Bitwise_shift_operators) `i << j` is defined as <em>i * 2<sup>j</su
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913521985)
In commit "util: Add integer left shift helpers" (e0a541702def3a4c6cce8e44b6ebe8d608edad4e)
It seems wrong that calling `SaturatingLeftShift` on a negative input can wrap around to 0. The point of saturating operations should be that they don't wrap around, but effectively perform operations with full precision and just clamp the result values. In c++ [left shift](https://en.cppreference.com/w/cpp/language/operator_arithmetic#Bitwise_shift_operators) `i << j` is defined as <em>i * 2<sup>j</su
...
💬 fanquake commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2587704437)
> The root cause of the issue is that the -Wl,-z,separate-code linker option is incompatible with NetBSD's dynamic linker.
That's pretty surprising, because `-z,separate-code` has been enabled by default, in the NetBSD linker, for the upcoming 11.0 release. https://www.netbsd.org/changes/changes-11.0.html#x86:
> Enable the -z separate-code security feature by default in [ld(1)](https://man.netbsd.org/ld.1).
It seems kinda unlikely they'd do that if the option was incompatible with the
...
(https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2587704437)
> The root cause of the issue is that the -Wl,-z,separate-code linker option is incompatible with NetBSD's dynamic linker.
That's pretty surprising, because `-z,separate-code` has been enabled by default, in the NetBSD linker, for the upcoming 11.0 release. https://www.netbsd.org/changes/changes-11.0.html#x86:
> Enable the -z separate-code security feature by default in [ld(1)](https://man.netbsd.org/ld.1).
It seems kinda unlikely they'd do that if the option was incompatible with the
...
🤔 Sjors reviewed a pull request: "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases"
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2547082375)
Some questions in the form of feedback...
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2547082375)
Some questions in the form of feedback...
💬 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
...