π¬ TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088847208)
Sweet, will apply this!
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2088847208)
Sweet, will apply this!
π¬ Sjors commented on issue "Dropping unused legacy wallet code":
(https://github.com/bitcoin-core/gui/issues/873#issuecomment-2880140236)
I don't think I found all the code, regular GUI contributors might find more.
(https://github.com/bitcoin-core/gui/issues/873#issuecomment-2880140236)
I don't think I found all the code, regular GUI contributors might find more.
π¬ maflcko commented on issue "Dropping unused legacy wallet code":
(https://github.com/bitcoin-core/gui/issues/873#issuecomment-2880157568)
I think they can just open a new issue or pull request, if they find something? I guess it can't hurt to open/close this issue as a notification, but absent any bug or any concrete work to do, I am not sure what needs to be done here and when this can be closed.
(https://github.com/bitcoin-core/gui/issues/873#issuecomment-2880157568)
I think they can just open a new issue or pull request, if they find something? I guess it can't hurt to open/close this issue as a notification, but absent any bug or any concrete work to do, I am not sure what needs to be done here and when this can be closed.
π l0rinc opened a pull request: "merkle: preβreserve leaves to prevent reallocs with odd vtx count"
(https://github.com/bitcoin/bitcoin/pull/32497)
### Summary
`ComputeMerkleRoot` duplicates the last hash when the input size is odd:
https://github.com/bitcoin/bitcoin/blob/39b6c139bd6be33699af781f1d71f6fed303d468/src/consensus/merkle.cpp#L54-L56
If the caller gave it a `std::vector` whose capacity equals its size (the common case when the vector was created with `resize()`), that single `push_back` forces a reallocation (doubling its size).
We pay this penalty on every Merkleβroot calculation even though we know the final size in advance
...
(https://github.com/bitcoin/bitcoin/pull/32497)
### Summary
`ComputeMerkleRoot` duplicates the last hash when the input size is odd:
https://github.com/bitcoin/bitcoin/blob/39b6c139bd6be33699af781f1d71f6fed303d468/src/consensus/merkle.cpp#L54-L56
If the caller gave it a `std::vector` whose capacity equals its size (the common case when the vector was created with `resize()`), that single `push_back` forces a reallocation (doubling its size).
We pay this penalty on every Merkleβroot calculation even though we know the final size in advance
...
π¬ ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#issuecomment-2880169560)
@Sjors I've taken your suggestion in this push [0d774ab59..62fc42d475](https://github.com/bitcoin/bitcoin/compare/51344920ebe1772d8dd3bece789a6510d774ab59..62fc42d475df4f23bd93313f95ee7b7eb0d4683f)
If the `GetTip` method from the mining interface isn't going to be used by the TP, it can be deleted as well?
(https://github.com/bitcoin/bitcoin/pull/32378#issuecomment-2880169560)
@Sjors I've taken your suggestion in this push [0d774ab59..62fc42d475](https://github.com/bitcoin/bitcoin/compare/51344920ebe1772d8dd3bece789a6510d774ab59..62fc42d475df4f23bd93313f95ee7b7eb0d4683f)
If the `GetTip` method from the mining interface isn't going to be used by the TP, it can be deleted as well?
π¬ Sjors commented on issue "Dropping unused legacy wallet code":
(https://github.com/bitcoin-core/gui/issues/873#issuecomment-2880178818)
In that case I can close this once https://github.com/bitcoin/bitcoin/pull/32459 is merged, unless there's another issue found by then.
(https://github.com/bitcoin-core/gui/issues/873#issuecomment-2880178818)
In that case I can close this once https://github.com/bitcoin/bitcoin/pull/32459 is merged, unless there's another issue found by then.
π¬ l0rinc commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2088900114)
we could, but I would be mixing changes in the same commit - the commit message wouldn't represent the behavior. Or you mean I should just squash the last few changes as "(un)serialization consistency"?
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2088900114)
we could, but I would be mixing changes in the same commit - the commit message wouldn't represent the behavior. Or you mean I should just squash the last few changes as "(un)serialization consistency"?
π¬ instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2088901333)
Directionally I want them marked for removal, even if it sits around for a few years. This can re-litigated with new information perhaps, but I don't find it "honest" to not mark it as such unless the project changes direction.
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2088901333)
Directionally I want them marked for removal, even if it sits around for a few years. This can re-litigated with new information perhaps, but I don't find it "honest" to not mark it as such unless the project changes direction.
π¬ maflcko commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2088912362)
You can just append a section to the commit message:
```
Also, fixup whitespace while touching...
```
I am just mentioning it, because I often use `git log -S 'string'` and it is tedious to walk back the history if `string` is modified three times just to fixup some whitespace.
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2088912362)
You can just append a section to the commit message:
```
Also, fixup whitespace while touching...
```
I am just mentioning it, because I often use `git log -S 'string'` and it is tedious to walk back the history if `string` is modified three times just to fixup some whitespace.
π¬ maflcko commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2880212546)
https://developercommunity.visualstudio.com/t/C1001:-Internal-compiler-error-with-st/10830350#T-N10903514 . So I guess this was fixed in Visual Studio 2022 v17.14?
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2880212546)
https://developercommunity.visualstudio.com/t/C1001:-Internal-compiler-error-with-st/10830350#T-N10903514 . So I guess this was fixed in Visual Studio 2022 v17.14?
π Sjors approved a pull request: "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner"
(https://github.com/bitcoin/bitcoin/pull/32378#pullrequestreview-2840153694)
ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f
We could do another round of interface pruning like #31196 closer to the v30 branch-off.
Although `getTip()` isn't used by the TP, it does seem generally useful and trivial to maintain. But we can decide later if that's a good enough reason.
(https://github.com/bitcoin/bitcoin/pull/32378#pullrequestreview-2840153694)
ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f
We could do another round of interface pruning like #31196 closer to the v30 branch-off.
Although `getTip()` isn't used by the TP, it does seem generally useful and trivial to maintain. But we can decide later if that's a good enough reason.
π¬ hebasto commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2880215710)
> https://developercommunity.visualstudio.com/t/C1001:-Internal-compiler-error-with-st/10830350#T-N10903514 . So I guess this was fixed in Visual Studio 2022 v17.14?
Correct.
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2880215710)
> https://developercommunity.visualstudio.com/t/C1001:-Internal-compiler-error-with-st/10830350#T-N10903514 . So I guess this was fixed in Visual Studio 2022 v17.14?
Correct.
π€ danielabrozzoni reviewed a pull request: "config: allow setting -proxy per network"
(https://github.com/bitcoin/bitcoin/pull/32425#pullrequestreview-2840193686)
Code Review ACK e98c51fcce9ae3f441a416cab32a5c85756c6c64
I have reviewed the code, and it looks good to me.
I run the tests locally and did some very limited manual testing, mimicking the functional test behavior (passing in `-proxy=...=network` and checking the `getnetworkinfo` output)
(https://github.com/bitcoin/bitcoin/pull/32425#pullrequestreview-2840193686)
Code Review ACK e98c51fcce9ae3f441a416cab32a5c85756c6c64
I have reviewed the code, and it looks good to me.
I run the tests locally and did some very limited manual testing, mimicking the functional test behavior (passing in `-proxy=...=network` and checking the `getnetworkinfo` output)
π¬ l0rinc commented on pull request "refactor: Remove UB in prevector reverse iterators":
(https://github.com/bitcoin/bitcoin/pull/32490#issuecomment-2880252363)
tested ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685
Thanks for fixing it properly - this explains why I couldn't trigger an actual failure (local or CI) in https://github.com/bitcoin/bitcoin/pull/32296.
(https://github.com/bitcoin/bitcoin/pull/32490#issuecomment-2880252363)
tested ACK faf9082a5f689e2e51a474bf654e4e9b6ca29685
Thanks for fixing it properly - this explains why I couldn't trigger an actual failure (local or CI) in https://github.com/bitcoin/bitcoin/pull/32296.
π¬ polespinasa commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2088973429)
I agree, want it deleted too. Just added this comment because of the discussions some core members were having yesterday on twitter regarding what deprecation means bc people were getting crazy saying it means removal.
But it's just an observation, feel free to ignore.
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2088973429)
I agree, want it deleted too. Just added this comment because of the discussions some core members were having yesterday on twitter regarding what deprecation means bc people were getting crazy saying it means removal.
But it's just an observation, feel free to ignore.
π¬ hodlinator commented on issue "qa: Failure in wallet_basic.py spendzeroconfchange test":
(https://github.com/bitcoin/bitcoin/issues/32456#issuecomment-2880297237)
@mzumsande thanks for fixing this! Sorry I didn't have the time to fully understand how the wallet syncs before the fix was merged.
(https://github.com/bitcoin/bitcoin/issues/32456#issuecomment-2880297237)
@mzumsande thanks for fixing this! Sorry I didn't have the time to fully understand how the wallet syncs before the fix was merged.
π¬ l0rinc commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2088990681)
Split into 2 commits now, minimizing the same line being changed multiple times.
Also dropped the `prevector` related change which you've fixed properly in https://github.com/bitcoin/bitcoin/pull/32490
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2088990681)
Split into 2 commits now, minimizing the same line being changed multiple times.
Also dropped the `prevector` related change which you've fixed properly in https://github.com/bitcoin/bitcoin/pull/32490
π¬ ryanofsky commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#issuecomment-2880338215)
All the points about drawbacks of addressing this issue with a comment ("I usually treat comments as admitting defeat") are well taken and substantive, but I think it is good to merge this PR anyway because it accurately points out a confusing behavior that could cause bugs. I don't think we need to admit defeat if we can clear the way for new approaches by not spending too much time debating this particular approach. So am planning to merge this shortly.
(https://github.com/bitcoin/bitcoin/pull/31624#issuecomment-2880338215)
All the points about drawbacks of addressing this issue with a comment ("I usually treat comments as admitting defeat") are well taken and substantive, but I think it is good to merge this PR anyway because it accurately points out a confusing behavior that could cause bugs. I don't think we need to admit defeat if we can clear the way for new approaches by not spending too much time debating this particular approach. So am planning to merge this shortly.
π ryanofsky merged a pull request: "doc: warn that CheckBlock() underestimates sigops"
(https://github.com/bitcoin/bitcoin/pull/31624)
(https://github.com/bitcoin/bitcoin/pull/31624)
π fanquake opened a pull request: "doc: remove Carls substitute server from Guix docs"
(https://github.com/bitcoin/bitcoin/pull/32498)
This no-longer exists. Use one of the other Guix servers in the example.
(https://github.com/bitcoin/bitcoin/pull/32498)
This no-longer exists. Use one of the other Guix servers in the example.