🤔 jonatack reviewed a pull request: "Removing Bitcoin core text where unnecessary"
(https://github.com/bitcoin/bitcoin/pull/33126#pullrequestreview-3094421132)
Concept ACK, provided this small patch is robust. It's a minor change to be sure. Nevertheless, it seems cleaner not to hardcode it and would convey a magnanimous FOSS spirit that I encourage.
(https://github.com/bitcoin/bitcoin/pull/33126#pullrequestreview-3094421132)
Concept ACK, provided this small patch is robust. It's a minor change to be sure. Nevertheless, it seems cleaner not to hardcode it and would convey a magnanimous FOSS spirit that I encourage.
💬 achow101 commented on pull request "refactor,test: follow-ups to multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/33039#issuecomment-3161767528)
ACK 86e3a0a8cbd30cfee98f5b4acf4ce6d0a75a3ef0
(https://github.com/bitcoin/bitcoin/pull/33039#issuecomment-3161767528)
ACK 86e3a0a8cbd30cfee98f5b4acf4ce6d0a75a3ef0
💬 hodlinator commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2258425819)
Haven't explored this in depth, but my guess is that the zip filename comes from this line in `add_macos_deploy_target`:
https://github.com/bitcoin/bitcoin/blob/2c9aa1afa8dab8cf2c86b10010f1d3b342d93425/cmake/module/Maintenance.cmake#L87
An alternative approach might be to change that to something generic like `CFBundleName = \"BundleName\"`, or make it have something closer to the final name in the first place.
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2258425819)
Haven't explored this in depth, but my guess is that the zip filename comes from this line in `add_macos_deploy_target`:
https://github.com/bitcoin/bitcoin/blob/2c9aa1afa8dab8cf2c86b10010f1d3b342d93425/cmake/module/Maintenance.cmake#L87
An alternative approach might be to change that to something generic like `CFBundleName = \"BundleName\"`, or make it have something closer to the final name in the first place.
💬 murchandamus commented on issue "Slow unit tests delay functional tests and leave CPU unused":
(https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-3161801097)
In the context of `coinselector_tests` being slow, I found looking at some flamegraphs that most of the time is spent on Knapsack and BnB. I have a [rewrite of BnB](https://github.com/bitcoin/bitcoin/pull/32150) open, that I would expect to improve BnB’s performance (and thus improve this test’s performance, in case someone is interested to review) and I have been prototyping another coin selection algorithm that I hope will bring the last missing piece to obsolete Knapsack (consolidatory behavi
...
(https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-3161801097)
In the context of `coinselector_tests` being slow, I found looking at some flamegraphs that most of the time is spent on Knapsack and BnB. I have a [rewrite of BnB](https://github.com/bitcoin/bitcoin/pull/32150) open, that I would expect to improve BnB’s performance (and thus improve this test’s performance, in case someone is interested to review) and I have been prototyping another coin selection algorithm that I hope will bring the last missing piece to obsolete Knapsack (consolidatory behavi
...
🤔 achow101 reviewed a pull request: "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key"
(https://github.com/bitcoin/bitcoin/pull/32471#pullrequestreview-3094495318)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32471#pullrequestreview-3094495318)
Concept ACK
💬 achow101 commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2258450650)
In 49f41141c674f83d0b66262a5b21aad623704264 "descriptor: ToPrivateString() pass if at least 1 priv key exists"
`has_priv_key` should be a reference not a pointer. The pointer-ness is never used in this function.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2258450650)
In 49f41141c674f83d0b66262a5b21aad623704264 "descriptor: ToPrivateString() pass if at least 1 priv key exists"
`has_priv_key` should be a reference not a pointer. The pointer-ness is never used in this function.
💬 achow101 commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2258440013)
In 64213213b3691c9d93de95f880f820a640cd3c35 "descriptors: add IsWatchOnly()"
I think this function can be vastly simplified by calling `GetPrivKey` on all the `PubkeyProvider`, and `IsWatchOnly` on all subdescriptors.
nit: I don't like this function name since whether the descriptor is watchonly depends on the contents of the provided parameter. It's not a property like a `Is*` name implies. Watchonly also is not a property applied to descriptors as private keys are not actually part of th
...
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2258440013)
In 64213213b3691c9d93de95f880f820a640cd3c35 "descriptors: add IsWatchOnly()"
I think this function can be vastly simplified by calling `GetPrivKey` on all the `PubkeyProvider`, and `IsWatchOnly` on all subdescriptors.
nit: I don't like this function name since whether the descriptor is watchonly depends on the contents of the provided parameter. It's not a property like a `Is*` name implies. Watchonly also is not a property applied to descriptors as private keys are not actually part of th
...
💬 achow101 commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2258454050)
In 49f41141c674f83d0b66262a5b21aad623704264 "descriptor: ToPrivateString() pass if at least 1 priv key exists"
`assert`s should not have side effects. If we are calling a function to actually do something, it should not be in an `assert`.
However, if the return value of `ToStringHelper` is always supposed to be `true`, then it should not be returning anything anyways. Furthermore, I don't see why `ToStringHelper` doesn't return whether the string has a private key, as we are doing in this
...
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2258454050)
In 49f41141c674f83d0b66262a5b21aad623704264 "descriptor: ToPrivateString() pass if at least 1 priv key exists"
`assert`s should not have side effects. If we are calling a function to actually do something, it should not be in an `assert`.
However, if the return value of `ToStringHelper` is always supposed to be `true`, then it should not be returning anything anyways. Furthermore, I don't see why `ToStringHelper` doesn't return whether the string has a private key, as we are doing in this
...
💬 achow101 commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2258451488)
In 49f41141c674f83d0b66262a5b21aad623704264 "descriptor: ToPrivateString() pass if at least 1 priv key exists"
If `has_priv_key` is never supposed to be null, then it should be a reference. Otherwise, this should be an `if`, we should not rely on caller behavior.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2258451488)
In 49f41141c674f83d0b66262a5b21aad623704264 "descriptor: ToPrivateString() pass if at least 1 priv key exists"
If `has_priv_key` is never supposed to be null, then it should be a reference. Otherwise, this should be an `if`, we should not rely on caller behavior.
🚀 achow101 merged a pull request: "refactor,test: follow-ups to multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/33039)
(https://github.com/bitcoin/bitcoin/pull/33039)
💬 w0xlt commented on pull request "Musig2 tests":
(https://github.com/bitcoin/bitcoin/pull/32724#issuecomment-3161888573)
> Just two test vectors?
Yes, they cover BIP 328 test vectors and the mutation detected by @brunoerg .
BIP 327 doesn't have any test vectors (even the musig2 secp256k1 library uses random keys in the test).
BIP 328 test vectors are particularly interesting because they test aggregate keys and xpubs.
(https://github.com/bitcoin/bitcoin/pull/32724#issuecomment-3161888573)
> Just two test vectors?
Yes, they cover BIP 328 test vectors and the mutation detected by @brunoerg .
BIP 327 doesn't have any test vectors (even the musig2 secp256k1 library uses random keys in the test).
BIP 328 test vectors are particularly interesting because they test aggregate keys and xpubs.
🤔 l0rinc reviewed a pull request: "[IBD] flush UTXO set in batches proportional to `dbcache` size"
(https://github.com/bitcoin/bitcoin/pull/31645#pullrequestreview-3094683562)
Pushed the remaining nits that I promised a long time ago :)
Re-rebased, you can re-review with `git range-diff af653f3...956a6b4`
(https://github.com/bitcoin/bitcoin/pull/31645#pullrequestreview-3094683562)
Pushed the remaining nits that I promised a long time ago :)
Re-rebased, you can re-review with `git range-diff af653f3...956a6b4`
💬 l0rinc commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2258580736)
Ended up with extracting min/max and adding an assert
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2258580736)
Ended up with extracting min/max and adding an assert
💬 l0rinc commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2258579061)
Added braces
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2258579061)
Added braces
💬 l0rinc commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2258579473)
Extracted min/max and added some comments
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2258579473)
Extracted min/max and added some comments
💬 l0rinc commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2258579567)
Done
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2258579567)
Done
💬 l0rinc commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3162052400)
I'm not sure why it would be our job to make the code more forkable - but we do need to make it maintainable, so concept ACK for less hard-coding - no opinion on the implementation details, will let others review the specifics.
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3162052400)
I'm not sure why it would be our job to make the code more forkable - but we do need to make it maintainable, so concept ACK for less hard-coding - no opinion on the implementation details, will let others review the specifics.
📝 bethoffman opened a pull request: "doc:Fix typos"
(https://github.com/bitcoin/bitcoin/pull/33148)
Minor PR to fix spelling
(https://github.com/bitcoin/bitcoin/pull/33148)
Minor PR to fix spelling
💬 l0rinc commented on pull request "doc:Fix typos":
(https://github.com/bitcoin/bitcoin/pull/33148#discussion_r2258736822)
This was already proposed upstream at https://github.com/google/leveldb/pull/769 and https://github.com/google/leveldb/pull/739/files - but since that repo is basically dead, you can propose it to our fork in https://github.com/bitcoin-core/leveldb-subtree instead (where it will also likely be ignored since there's an opportunity cost for spending time with these)
(https://github.com/bitcoin/bitcoin/pull/33148#discussion_r2258736822)
This was already proposed upstream at https://github.com/google/leveldb/pull/769 and https://github.com/google/leveldb/pull/739/files - but since that repo is basically dead, you can propose it to our fork in https://github.com/bitcoin-core/leveldb-subtree instead (where it will also likely be ignored since there's an opportunity cost for spending time with these)
🤔 octavio12345300 reviewed a pull request: "doc:Fix typos"
(https://github.com/bitcoin/bitcoin/pull/33148#pullrequestreview-3094980543)
LIST
(https://github.com/bitcoin/bitcoin/pull/33148#pullrequestreview-3094980543)
LIST