💬 Sjors commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1927009876)
Concept ACK. Moving things out of util that the kernel will / should never need, makes sense to me. I don't have strong feelings on where exactly to move these things to.
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1927009876)
Concept ACK. Moving things out of util that the kernel will / should never need, makes sense to me. I don't have strong feelings on where exactly to move these things to.
💬 josibake commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1927058987)
> I think the newly added generic ser/unser methods to CKey are harmless.
Its adding a footgun to `CKey`. The class is making sure to keep the secret data secure in memory and to free it when the key is gone, so allowing the key to be created directly from (potentially insecure) data on disk and allowing the key to be written out unencrypted to disk without destroying the key seems like an anti-pattern. You could argue that the existing methods (e.g. `GetPrivKey`) are also footguns, but that
...
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1927058987)
> I think the newly added generic ser/unser methods to CKey are harmless.
Its adding a footgun to `CKey`. The class is making sure to keep the secret data secure in memory and to free it when the key is gone, so allowing the key to be created directly from (potentially insecure) data on disk and allowing the key to be written out unencrypted to disk without destroying the key seems like an anti-pattern. You could argue that the existing methods (e.g. `GetPrivKey`) are also footguns, but that
...
🤔 glozow reviewed a pull request: "test: Assumeutxo with more than just coinbase transactions"
(https://github.com/bitcoin/bitcoin/pull/29354#pullrequestreview-1862914988)
concept ACK fa5cd66f0a47d1b759c93d01524ee4558432c0cc
(https://github.com/bitcoin/bitcoin/pull/29354#pullrequestreview-1862914988)
concept ACK fa5cd66f0a47d1b759c93d01524ee4558432c0cc
🚀 glozow merged a pull request: "test: Assumeutxo with more than just coinbase transactions"
(https://github.com/bitcoin/bitcoin/pull/29354)
(https://github.com/bitcoin/bitcoin/pull/29354)
🚀 glozow merged a pull request: "log: Don't use scientific notation in log messages"
(https://github.com/bitcoin/bitcoin/pull/29254)
(https://github.com/bitcoin/bitcoin/pull/29254)
🤔 furszy reviewed a pull request: "wallet: batch and simplify addressbook migration process"
(https://github.com/bitcoin/bitcoin/pull/26836#pullrequestreview-1862770273)
Updated per feedback, thanks for the review josie!
(https://github.com/bitcoin/bitcoin/pull/26836#pullrequestreview-1862770273)
Updated per feedback, thanks for the review josie!
💬 furszy commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1478234209)
Because I made this commit before e83babe (#27217), which removed the `IsMine` calls, and I forgot to update the commit description after the rebase. Will clean it, thanks.
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1478234209)
Because I made this commit before e83babe (#27217), which removed the `IsMine` calls, and I forgot to update the commit description after the rebase. Will clean it, thanks.
💬 furszy commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1478266195)
As we will never merge a PR solely removing the extra parentheses, we can either clean them now while working on this function or we keep them for another decade. I prefer to clean them now, but np in reverting the line if it conflicts with the review process.
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1478266195)
As we will never merge a PR solely removing the extra parentheses, we can either clean them now while working on this function or we keep them for another decade. I prefer to clean them now, but np in reverting the line if it conflicts with the review process.
💬 furszy commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1478340011)
> If the destination requires_transfer but isn't ours, go to the next wallet. If this happens for both wallets, copy will never be set and we will fail
If the destination requires transfer (a "receive" addr that is no longer part of the main wallet), the process checks to which wallet the entry belongs to. And, when the entry does not belong to any of the created wallets, it means that the key/script-to-descriptor migration process failed. This is because "receive" entries must always be trac
...
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1478340011)
> If the destination requires_transfer but isn't ours, go to the next wallet. If this happens for both wallets, copy will never be set and we will fail
If the destination requires transfer (a "receive" addr that is no longer part of the main wallet), the process checks to which wallet the entry belongs to. And, when the entry does not belong to any of the created wallets, it means that the key/script-to-descriptor migration process failed. This is because "receive" entries must always be trac
...
💬 furszy commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1478241812)
sure
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1478241812)
sure
💬 furszy commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1478350465)
sure
(https://github.com/bitcoin/bitcoin/pull/26836#discussion_r1478350465)
sure
✅ glozow closed a pull request: "doc: update comment in policy.cpp to refer to virtual bytes"
(https://github.com/bitcoin/bitcoin/pull/27726)
(https://github.com/bitcoin/bitcoin/pull/27726)
💬 glozow commented on pull request "doc: update comment in policy.cpp to refer to virtual bytes":
(https://github.com/bitcoin/bitcoin/pull/27726#issuecomment-1927156565)
Closing for lack of activity + too trivial.
(https://github.com/bitcoin/bitcoin/pull/27726#issuecomment-1927156565)
Closing for lack of activity + too trivial.
💬 GBKS commented on pull request "Change address / amount error background":
(https://github.com/bitcoin-core/gui/pull/553#issuecomment-1927157744)
ACK fe7c81e
Just got the DrahtBot notification and realized I was asked for my input. Sorry, for the delay on this. I agree with this change. The text remains more legible. Also visually less jarring with the outline.
If one wanted to keep a red background, it could also be a red with transparency (15-25% might be enough). That would avoid the thick border, still draw enough attention, and not impact contrast so much.
(https://github.com/bitcoin-core/gui/pull/553#issuecomment-1927157744)
ACK fe7c81e
Just got the DrahtBot notification and realized I was asked for my input. Sorry, for the delay on this. I agree with this change. The text remains more legible. Also visually less jarring with the outline.
If one wanted to keep a red background, it could also be a red with transparency (15-25% might be enough). That would avoid the thick border, still draw enough attention, and not impact contrast so much.
💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1927160569)
> It also seems strange to me that all of this care is being taken to keep the secret data secure in memory when you are reading writing it unencrypted to and from disk.
That's literally what the wallet does by default. `CKey` provides extra security for free, so it's better to use it. The optional wallet encryption feature makes use of this security feature, by ensuring that once a wallet is (re)locked the key data is erased from memory.
It would take _additional_ code for Sv2 to handle t
...
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1927160569)
> It also seems strange to me that all of this care is being taken to keep the secret data secure in memory when you are reading writing it unencrypted to and from disk.
That's literally what the wallet does by default. `CKey` provides extra security for free, so it's better to use it. The optional wallet encryption feature makes use of this security feature, by ensuring that once a wallet is (re)locked the key data is erased from memory.
It would take _additional_ code for Sv2 to handle t
...
✅ maflcko closed a pull request: "p2p: ensure mapBlockSource is removed from in ProcessBlock"
(https://github.com/bitcoin/bitcoin/pull/28235)
(https://github.com/bitcoin/bitcoin/pull/28235)
💬 instagibbs commented on pull request "p2p: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1927184068)
@Crypt-iQ could you just open a parallel issue for this for tracking?
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1927184068)
@Crypt-iQ could you just open a parallel issue for this for tracking?
💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1927200215)
Also, it would be good to include the test to ensure the crash no longer happens.
https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1918947945
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1927200215)
Also, it would be good to include the test to ensure the crash no longer happens.
https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1918947945
💬 theuni commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1927224463)
I've pushed a commit which temporarily puts the includes back in the low-level headers for the sake of addressing the benchmarks first, setting aside the possibility of missing defines.
@aureleoules's [benchmarks show 2 small regressions](https://corecheck.dev/bitcoin/bitcoin/pulls/29263), though I am unable to reproduce those locally.
My tests show: `./bench/bench_bitcoin -min-time=5000 -filter="AddrManGetAddr|PrevectorDeserializeNontrivial"`
master (3d52cedb497e0ec029ba3cef95b00169c157d
...
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1927224463)
I've pushed a commit which temporarily puts the includes back in the low-level headers for the sake of addressing the benchmarks first, setting aside the possibility of missing defines.
@aureleoules's [benchmarks show 2 small regressions](https://corecheck.dev/bitcoin/bitcoin/pulls/29263), though I am unable to reproduce those locally.
My tests show: `./bench/bench_bitcoin -min-time=5000 -filter="AddrManGetAddr|PrevectorDeserializeNontrivial"`
master (3d52cedb497e0ec029ba3cef95b00169c157d
...
💬 vasild commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-1927234971)
Then I misunderstood this:
> This class could Assume that the file was flushed/closed before the destructor is called
How?
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-1927234971)
Then I misunderstood this:
> This class could Assume that the file was flushed/closed before the destructor is called
How?