💬 l0rinc commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2095953423)
... and I think it should be a `size_t` instead
(https://github.com/bitcoin/bitcoin/pull/32530#discussion_r2095953423)
... and I think it should be a `size_t` instead
💬 maflcko commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2096016006)
> Then again, some people are just forking the project on master and tag some past version on it. That could be equally confusing, no?
Hopefully that is rare. Though, just including the full commit hash in bug reports shouldn't hurt.
(https://github.com/bitcoin/bitcoin/pull/32543#discussion_r2096016006)
> Then again, some people are just forking the project on master and tag some past version on it. That could be equally confusing, no?
Hopefully that is rare. Though, just including the full commit hash in bug reports shouldn't hurt.
💬 fanquake commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#issuecomment-2891504575)
Concept ACK. Seems like there is some overlap here with #32380, but that this PR should go first, otherwise #32380 is going to be refactoring code, that is being deleted here.
(https://github.com/bitcoin/bitcoin/pull/32566#issuecomment-2891504575)
Concept ACK. Seems like there is some overlap here with #32380, but that this PR should go first, otherwise #32380 is going to be refactoring code, that is being deleted here.
💬 stickies-v commented on pull request "doc: remove // for ... comments":
(https://github.com/bitcoin/bitcoin/pull/32562#issuecomment-2891504937)
re-ACK 7193245cd66791216d4e586ca09302b26d4b7528
(https://github.com/bitcoin/bitcoin/pull/32562#issuecomment-2891504937)
re-ACK 7193245cd66791216d4e586ca09302b26d4b7528
💬 fanquake commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2096035158)
Is anything enforcing this, or is the assumption that any Windows 10 system is this at least version? What happens if it isn't?
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2096035158)
Is anything enforcing this, or is the assumption that any Windows 10 system is this at least version? What happens if it isn't?
💬 hebasto commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2096044476)
> Is anything enforcing this, or is the assumption that any Windows 10 system is at least this version?
The latter.
> What happens if it isn't?
My guess is that the application won't properly handle any symbols outside its default code page.
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2096044476)
> Is anything enforcing this, or is the assumption that any Windows 10 system is at least this version?
The latter.
> What happens if it isn't?
My guess is that the application won't properly handle any symbols outside its default code page.
💬 ryanofsky commented on pull request "Mining: Avoid copying template CBlocks":
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2891540520)
I think goals of this PR should be achievable without too much work, but unfortunately the implementation as of 39a3b5be3d138c8f3f7cc4d5bd22c1bd42f4d525 isn't really compatible with IPC so a different approach needs to be taken. There are two problems currently:
1. First, the pure virtual methods cannot be marked const. In general it doesn't make sense to mark virtual methods const, because parent classes can't know how subclasses will implement the virtual methods and what state they may acc
...
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2891540520)
I think goals of this PR should be achievable without too much work, but unfortunately the implementation as of 39a3b5be3d138c8f3f7cc4d5bd22c1bd42f4d525 isn't really compatible with IPC so a different approach needs to be taken. There are two problems currently:
1. First, the pure virtual methods cannot be marked const. In general it doesn't make sense to mark virtual methods const, because parent classes can't know how subclasses will implement the virtual methods and what state they may acc
...
💬 stickies-v commented on pull request "Remove legacy `Parse(U)Int*`":
(https://github.com/bitcoin/bitcoin/pull/32520#issuecomment-2891542065)
re-ACK 33332829333b589420f8038541d04ec6970f051d
(https://github.com/bitcoin/bitcoin/pull/32520#issuecomment-2891542065)
re-ACK 33332829333b589420f8038541d04ec6970f051d
💬 laanwj commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#issuecomment-2891561571)
i'm not 100% sure `ENABLE_SUBPROCESS` needs to exist as an option at all, btw, it may be something to always enable, but maybe not on mobile/sandboxed platforms so wasn't sure whether to make that decision here.
(https://github.com/bitcoin/bitcoin/pull/32566#issuecomment-2891561571)
i'm not 100% sure `ENABLE_SUBPROCESS` needs to exist as an option at all, btw, it may be something to always enable, but maybe not on mobile/sandboxed platforms so wasn't sure whether to make that decision here.
💬 l0rinc commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2096072843)
> I can't see any
Most usages have a hash available, but I'm not sure about e.g. [`LoadExternalBlockFile`](https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L5166) - but even if I would, I'd prefer tackling it separately, I'm not comfortable adding new exceptions to consensus code.
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2096072843)
> I can't see any
Most usages have a hash available, but I'm not sure about e.g. [`LoadExternalBlockFile`](https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L5166) - but even if I would, I'd prefer tackling it separately, I'm not comfortable adding new exceptions to consensus code.
💬 hebasto commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#issuecomment-2891585620)
Concept ACK.
> Seems like there is some overlap here with #32380, but that this PR should go first, otherwise #32380 is going to be refactoring code, that is being deleted here.
I agree. And we are not going to switch to C++26 any time soon when #32380 will be required.
(https://github.com/bitcoin/bitcoin/pull/32566#issuecomment-2891585620)
Concept ACK.
> Seems like there is some overlap here with #32380, but that this PR should go first, otherwise #32380 is going to be refactoring code, that is being deleted here.
I agree. And we are not going to switch to C++26 any time soon when #32380 will be required.
✅ ryanofsky closed an issue: "rpc: actually deprecate `rpcuser` & `rpcpass`"
(https://github.com/bitcoin/bitcoin/issues/29240)
(https://github.com/bitcoin/bitcoin/issues/29240)
🚀 ryanofsky merged a pull request: "rpc: Undeprecate rpcuser/rpcpassword, store all credentials hashed in memory"
(https://github.com/bitcoin/bitcoin/pull/32423)
(https://github.com/bitcoin/bitcoin/pull/32423)
💬 theStack commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#issuecomment-2891709530)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32566#issuecomment-2891709530)
Concept ACK
👍 willcl-ark approved a pull request: "doc: remove // for ... comments"
(https://github.com/bitcoin/bitcoin/pull/32562#pullrequestreview-2851493004)
reACK 7193245cd66791216d4e586ca09302b26d4b7528
(https://github.com/bitcoin/bitcoin/pull/32562#pullrequestreview-2851493004)
reACK 7193245cd66791216d4e586ca09302b26d4b7528
💬 willcl-ark commented on issue "build: (initial?) failure to build xproto on Alpine":
(https://github.com/bitcoin/bitcoin/issues/32494#issuecomment-2891734846)
This appears to be related to make parallelism. It can be circumvented by using `make -j1`.
Another approach might be to patch the (failing) `mkdir` call with `mkdir -p`, although this might possibly also just mask underlying issue...
(https://github.com/bitcoin/bitcoin/issues/32494#issuecomment-2891734846)
This appears to be related to make parallelism. It can be circumvented by using `make -j1`.
Another approach might be to patch the (failing) `mkdir` call with `mkdir -p`, although this might possibly also just mask underlying issue...
💬 hebasto commented on pull request "Use WitnessV0KeyHash in TestAddAddressesToSendBook":
(https://github.com/bitcoin-core/gui/pull/875#issuecomment-2891735393)
cc @furszy
(https://github.com/bitcoin-core/gui/pull/875#issuecomment-2891735393)
cc @furszy
💬 Sjors commented on pull request "Mining: Avoid copying template CBlocks":
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2891771230)
> All these things together suggest to me that the Mining interface should probably be exposing the CBlockTemplate struct directly and not hiding it behind so many accessor methods.
The original reason for this was so that a Stratum v2 IPC sidecar can the minimal info it needs for a [NewTemplate](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#72-newtemplate-server---client) message: basically the version field, some parts of the coinbase and its mer
...
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2891771230)
> All these things together suggest to me that the Mining interface should probably be exposing the CBlockTemplate struct directly and not hiding it behind so many accessor methods.
The original reason for this was so that a Stratum v2 IPC sidecar can the minimal info it needs for a [NewTemplate](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#72-newtemplate-server---client) message: basically the version field, some parts of the coinbase and its mer
...
💬 Sjors commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2891787021)
> So I don't think it should be documented as if it were.
Indeed, this is a bit of a hack that happens to work nicely with our wallet design. If other wallets end up using it too, we could reconsider that.
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2891787021)
> So I don't think it should be documented as if it were.
Indeed, this is a bit of a hack that happens to work nicely with our wallet design. If other wallets end up using it too, we could reconsider that.
💬 achow101 commented on pull request "Use WitnessV0KeyHash in TestAddAddressesToSendBook":
(https://github.com/bitcoin-core/gui/pull/875#issuecomment-2891795645)
ACK fa982f14254433a969499e93c1c3f0db31dce6ab
Neither `i` nor `o` are part of the bech32 character set, so that should resolve this issue. It might be better to make the label something more unique though.
(https://github.com/bitcoin-core/gui/pull/875#issuecomment-2891795645)
ACK fa982f14254433a969499e93c1c3f0db31dce6ab
Neither `i` nor `o` are part of the bech32 character set, so that should resolve this issue. It might be better to make the label something more unique though.