💬 luke-jr commented on pull request "wallet: allow lable for external descriptor & disallow label for ranged descriptors":
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-2588907624)
It would be nice to have a test that checks all 4 success/failure cases.
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-2588907624)
It would be nice to have a test that checks all 4 success/failure cases.
💬 naiyoma commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1914319868)
I've moved the new user to the strange_user list, renamed user3 to strangedude6 for consistency, and replaced "" with None as suggested here https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1904497634
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1914319868)
I've moved the new user to the strange_user list, renamed user3 to strangedude6 for consistency, and replaced "" with None as suggested here https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1904497634
💬 naiyoma commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1914324487)
https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1914319868
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1914324487)
https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1914319868
💬 naiyoma commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1914326733)
Done https://github.com/bitcoin/bitcoin/pull/29858/commits/cfce5f5cbacf7d4c209112ac9d35856bed4a3c6b
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1914326733)
Done https://github.com/bitcoin/bitcoin/pull/29858/commits/cfce5f5cbacf7d4c209112ac9d35856bed4a3c6b
💬 naiyoma commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#issuecomment-2589165785)
> Code review ACK [fcf0ead](https://github.com/bitcoin/bitcoin/commit/fcf0ead6cd358fd35909e1102bdae2c176b0ace6). I left suggestions below, but none are important and they can be ignored. I think it is better to have this test coverage for this than not to have it, and no need to spend too much time iterating on the PR just to improve test code quality which is already decent.
Thanks for the review, I have pushed an update and resolved all the suggestions, https://github.com/bitcoin/bitcoin/
...
(https://github.com/bitcoin/bitcoin/pull/29858#issuecomment-2589165785)
> Code review ACK [fcf0ead](https://github.com/bitcoin/bitcoin/commit/fcf0ead6cd358fd35909e1102bdae2c176b0ace6). I left suggestions below, but none are important and they can be ignored. I think it is better to have this test coverage for this than not to have it, and no need to spend too much time iterating on the PR just to improve test code quality which is already decent.
Thanks for the review, I have pushed an update and resolved all the suggestions, https://github.com/bitcoin/bitcoin/
...
💬 vasild commented on pull request "test: avoid internet traffic":
(https://github.com/bitcoin/bitcoin/pull/31646#issuecomment-2589289729)
`66dbfaca22...2ed161c5ce`: do https://github.com/bitcoin/bitcoin/pull/31646#discussion_r1913933802
(https://github.com/bitcoin/bitcoin/pull/31646#issuecomment-2589289729)
`66dbfaca22...2ed161c5ce`: do https://github.com/bitcoin/bitcoin/pull/31646#discussion_r1913933802
💬 vasild commented on pull request "test: avoid internet traffic":
(https://github.com/bitcoin/bitcoin/pull/31646#discussion_r1914418707)
Indeed! Done, thanks!
(https://github.com/bitcoin/bitcoin/pull/31646#discussion_r1914418707)
Indeed! Done, thanks!
💬 hodlinator commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2589299296)
(I did a second run on 433412fd8478923dfdb20044f74c5d1e19fa8dd8, it took 3h45m53s, 9.5 minutes / 4.4% faster. So there is some variance).
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2589299296)
(I did a second run on 433412fd8478923dfdb20044f74c5d1e19fa8dd8, it took 3h45m53s, 9.5 minutes / 4.4% faster. So there is some variance).
💬 l0rinc commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2589302015)
These are the fastest IBDs I've seen so far
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2589302015)
These are the fastest IBDs I've seen so far
💬 Sjors commented on pull request "miner: always treat SegWit as active":
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2589341922)
It seems useful to illustrate the change here by regenerating the alternative mainnet blocks introduced in #31583, switching their coinbase outputs to taproot addresses. Before this PR the last block, which spends a coinbase output, would fail with `unexpected-witness`.
Marking as draft when that lands.
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2589341922)
It seems useful to illustrate the change here by regenerating the alternative mainnet blocks introduced in #31583, switching their coinbase outputs to taproot addresses. Before this PR the last block, which spends a coinbase output, would fail with `unexpected-witness`.
Marking as draft when that lands.
📝 Sjors converted_to_draft a pull request: "miner: always treat SegWit as active"
(https://github.com/bitcoin/bitcoin/pull/31625)
The `getblocktemplate` RPC would check if SegWit has activated yet at the tip. This has been the case for more than seven years.
(https://github.com/bitcoin/bitcoin/pull/31625)
The `getblocktemplate` RPC would check if SegWit has activated yet at the tip. This has been the case for more than seven years.
💬 hebasto commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2589347934)
> > 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 incompati
...
(https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2589347934)
> > 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 incompati
...
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1914459164)
dec5f8ba8ff579e3e708079dde8a7ff732884479: @maflcko I'm puzzled by the need to `deepcopy` here, but if I just do `block.vtx = txs` then in the next call to `mine`, without passing any transactions in, `txs` will have the coinbase of the previous block in it.
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1914459164)
dec5f8ba8ff579e3e708079dde8a7ff732884479: @maflcko I'm puzzled by the need to `deepcopy` here, but if I just do `block.vtx = txs` then in the next call to `mine`, without passing any transactions in, `txs` will have the coinbase of the previous block in it.
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1914465891)
Well the linter forced me to change the `txs` default to `None`, which "solved" the issue.
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1914465891)
Well the linter forced me to change the `txs` default to `None`, which "solved" the issue.
💬 hebasto commented on pull request "refactor: Enforce readability-avoid-const-params-in-decls":
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2589365958)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2589365958)
Concept ACK.
💬 maflcko commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1914467060)
I guess you figured it out, given the CI failure https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2589364296 ?
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1914467060)
I guess you figured it out, given the CI failure https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2589364296 ?
💬 NicolaLS commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1914473845)
thanks, I was wondering if there should be a blank line. for me it shows the table correctly (firefox, gh mobile) so I thought when in doubt just keep it compact.
weird that it displays correctly for me but not for you though (also tested with `gh-markdown-preview` locally).
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1914473845)
thanks, I was wondering if there should be a blank line. for me it shows the table correctly (firefox, gh mobile) so I thought when in doubt just keep it compact.
weird that it displays correctly for me but not for you though (also tested with `gh-markdown-preview` locally).
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1914478423)
> Your suggested implementation LGTM but I think keeping std::numeric_limits<T>::digits instead of sizeof(T) * CHAR_BIT is preferable since it takes into account the sign bit?
As far as I understand it is legal to shift e.g. `int32_t{1} << 31` to get the minimum `int32_t` value. But I think in this case we should prohibit this quirk of the underlying bit representation and use `std::numeric_limits<T>::digits`.
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1914478423)
> Your suggested implementation LGTM but I think keeping std::numeric_limits<T>::digits instead of sizeof(T) * CHAR_BIT is preferable since it takes into account the sign bit?
As far as I understand it is legal to shift e.g. `int32_t{1} << 31` to get the minimum `int32_t` value. But I think in this case we should prohibit this quirk of the underlying bit representation and use `std::numeric_limits<T>::digits`.
💬 NicolaLS commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589387132)
added a blank line between the paragraph and the table in `## Compiler` and moved `Python` to the `## Optional` table. thanks for the help everyone.
I think `Linux Kernel` should be moved to optional as well then but since it was in the required table before and I'm not sure if I should change more stuff after receiving acks I just left it as is now.
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589387132)
added a blank line between the paragraph and the table in `## Compiler` and moved `Python` to the `## Optional` table. thanks for the help everyone.
I think `Linux Kernel` should be moved to optional as well then but since it was in the required table before and I'm not sure if I should change more stuff after receiving acks I just left it as is now.
💬 hebasto commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2589399613)
> @hebasto: Did you get a reply in the Developer Community issue?
Not really. I only received an email confirming the creation of a new issue, along with the link I posted above.
> 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!
It's strange, but the link I received doesn't work for me either.
cc @CaseyCarter
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2589399613)
> @hebasto: Did you get a reply in the Developer Community issue?
Not really. I only received an email confirming the creation of a new issue, along with the link I posted above.
> 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!
It's strange, but the link I received doesn't work for me either.
cc @CaseyCarter