📝 maflcko opened a pull request: "rpc: Remove deprecated dummy alias for listtransactions::label"
(https://github.com/bitcoin/bitcoin/pull/31413)
The RPC arg is not a dummy, but a label, so offering an undocumented alias is confusing at best, if not entirely unused.
Fix it by removing the deprecated alias.
(https://github.com/bitcoin/bitcoin/pull/31413)
The RPC arg is not a dummy, but a label, so offering an undocumented alias is confusing at best, if not entirely unused.
Fix it by removing the deprecated alias.
💬 bigspider commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2514970384)
Thanks @Sjors for testing the app!
I just updated it with a new version (`Bitcoin Test Musig v2.4.0-rc`):
- Some bug fixes for more complex policies. Testing is still not extensive, but it should work for all combinations of musig and miniscript (with the due limits to the policy size; currently, at most 5 keys for MuSig2).
- If a psbt is sent where _only MuSig2 round 1_ is executed (no signatures returned), the app will return pubnonces _with no user interaction_. Note that if there are ot
...
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2514970384)
Thanks @Sjors for testing the app!
I just updated it with a new version (`Bitcoin Test Musig v2.4.0-rc`):
- Some bug fixes for more complex policies. Testing is still not extensive, but it should work for all combinations of musig and miniscript (with the due limits to the policy size; currently, at most 5 keys for MuSig2).
- If a psbt is sent where _only MuSig2 round 1_ is executed (no signatures returned), the app will return pubnonces _with no user interaction_. Note that if there are ot
...
🤔 hodlinator reviewed a pull request: "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2475934654)
Code reviewed f2fd1f7c043a2782cb2bf3c9fe7e2f94c17728b5
```
₿ git range-diff master 57caa96 f2fd1f7
₿ git show e314bb7e00 > old
₿ git show 91a8fde051 > new
₿ meld old new
```
Thanks for using more `constexpr` `std::array`s and clearer `sizeof`s!
Nice that block data could be compressed to such a large extent.
nit: Would prefer the *\*.cmake* changes where broken out into their own commit, keeping only *src/bench/CMakeLists.txt* as part of the benchmark change.
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2475934654)
Code reviewed f2fd1f7c043a2782cb2bf3c9fe7e2f94c17728b5
```
₿ git range-diff master 57caa96 f2fd1f7
₿ git show e314bb7e00 > old
₿ git show 91a8fde051 > new
₿ meld old new
```
Thanks for using more `constexpr` `std::array`s and clearer `sizeof`s!
Nice that block data could be compressed to such a large extent.
nit: Would prefer the *\*.cmake* changes where broken out into their own commit, keeping only *src/bench/CMakeLists.txt* as part of the benchmark change.
💬 hodlinator commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1867876958)
nit: Clearer name and source of `sizeof` expression.
```suggestion
for (constexpr auto chunk_size = sizeof(key); write.size() >= chunk_size; write = write.subspan(chunk_size)) {
XorInt(write, key, chunk_size);
```
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1867876958)
nit: Clearer name and source of `sizeof` expression.
```suggestion
for (constexpr auto chunk_size = sizeof(key); write.size() >= chunk_size; write = write.subspan(chunk_size)) {
XorInt(write, key, chunk_size);
```
💬 hodlinator commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1867892424)
Missing newline at EOF in latest version.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1867892424)
Missing newline at EOF in latest version.
💬 hodlinator commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1867942188)
nits:
Although there is precedent for adding references to gists, I'm not sure we should encourage it. Would have preferred a file in this repo's *contrib/* directory.
Would also have preferred that it did the full calculation of the .tgz file instead of having a hard-coded array, computing it by looking at linearalized blocks on disk.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1867942188)
nits:
Although there is precedent for adding references to gists, I'm not sure we should encourage it. Would have preferred a file in this repo's *contrib/* directory.
Would also have preferred that it did the full calculation of the .tgz file instead of having a hard-coded array, computing it by looking at linearalized blocks on disk.
💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1868020679)
9edbe5a48fe9a2c0d364882fcb7d44ae6aac6d24 before the latest push didn't have these `const` params. I think it was a mistake to add them in the latest push and would prefer them rolled back.
Another alternative is to discuss my samples of ratios of `const` value type params or why you didn't apply it to `AddFlags`.
Sorry to be annoying about this but for some reason it really annoys me. :)
Will take a timeout and see if I can let go of this, if you still insist.
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1868020679)
9edbe5a48fe9a2c0d364882fcb7d44ae6aac6d24 before the latest push didn't have these `const` params. I think it was a mistake to add them in the latest push and would prefer them rolled back.
Another alternative is to discuss my samples of ratios of `const` value type params or why you didn't apply it to `AddFlags`.
Sorry to be annoying about this but for some reason it really annoys me. :)
Will take a timeout and see if I can let go of this, if you still insist.
📝 instagibbs opened a pull request: "func: test orphan parent is re-requested from 2nd peer"
(https://github.com/bitcoin/bitcoin/pull/31414)
Small test which I couldn't find coverage for, and with the current test being a bit misleading in comments.
(https://github.com/bitcoin/bitcoin/pull/31414)
Small test which I couldn't find coverage for, and with the current test being a bit misleading in comments.
💬 darosior commented on pull request "Miner: never create a template which exploits the timewarp bug":
(https://github.com/bitcoin/bitcoin/pull/31376#discussion_r1868051679)
Because it is unnecessary?
(https://github.com/bitcoin/bitcoin/pull/31376#discussion_r1868051679)
Because it is unnecessary?
💬 achow101 commented on issue "macOS App Notarization & Stapling":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2515086614)
> I’m not sure how what you are explaining is in way different than the normal expected behavior? Code signing is done by the developer, and the notarization is done is done by Apple. I’ve been doing this third-party submission to Apple for a while now, so I can make stapled bitcoin binaries for my own use.
How is that the normal expected behavior? I've been through the as many docs on notarization as I could find and I don't think there was any mention of third parties being able to submit a
...
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-2515086614)
> I’m not sure how what you are explaining is in way different than the normal expected behavior? Code signing is done by the developer, and the notarization is done is done by Apple. I’ve been doing this third-party submission to Apple for a while now, so I can make stapled bitcoin binaries for my own use.
How is that the normal expected behavior? I've been through the as many docs on notarization as I could find and I don't think there was any mention of third parties being able to submit a
...
💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1868079220)
The codesigners are not necessarily maintainers. I don't think it's useful to make this broad when it has been extremely specific for codesigning for more than a decade.
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1868079220)
The codesigners are not necessarily maintainers. I don't think it's useful to make this broad when it has been extremely specific for codesigning for more than a decade.
💬 ryanofsky commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2515108628)
Concept ACK, but curious for more feedback from @maflcko about this PR. The actual code changes here do not seem too complicated but maybe they make the code less generic. I wonder if you think there are concrete downsides to this PR, or if the changes are ok but possibly not be worth the review effort (as https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2449824362 seems to suggest)
I'm happy to spend time reviewing this if it improves performance and doesn't cause other problems.
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2515108628)
Concept ACK, but curious for more feedback from @maflcko about this PR. The actual code changes here do not seem too complicated but maybe they make the code less generic. I wonder if you think there are concrete downsides to this PR, or if the changes are ok but possibly not be worth the review effort (as https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2449824362 seems to suggest)
I'm happy to spend time reviewing this if it improves performance and doesn't cause other problems.
...
📝 BrandonOdiwuor opened a pull request: "test: fix TestShell initialization and reset()"
(https://github.com/bitcoin/bitcoin/pull/31415)
Fixes TestShell initialization issues caused by resolving symlinks and looking for config.ini in the source path instead of the build path after migration to CMake (see https://github.com/bitcoin/bitcoin/issues/31131#issuecomment-2433056070)
https://github.com/bitcoin/bitcoin/blob/ebe4cac38bf6c510b00b8e080acab079c54016d6/test/functional/test_framework/test_shell.py#L77
also fixes https://github.com/bitcoin/bitcoin/issues/31131
(https://github.com/bitcoin/bitcoin/pull/31415)
Fixes TestShell initialization issues caused by resolving symlinks and looking for config.ini in the source path instead of the build path after migration to CMake (see https://github.com/bitcoin/bitcoin/issues/31131#issuecomment-2433056070)
https://github.com/bitcoin/bitcoin/blob/ebe4cac38bf6c510b00b8e080acab079c54016d6/test/functional/test_framework/test_shell.py#L77
also fixes https://github.com/bitcoin/bitcoin/issues/31131
💬 maflcko commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2515129785)
> Concept ACK, but curious for more feedback from @maflcko about this PR. The actual code changes here do not seem too complicated but maybe they make the code less generic.
There is a good chance that increasing the size of the vector is insufficient, if there is ever a need to increase it to more than 8 bytes, so a complete rewrite may be needed in that case anyway. However, this is just my guess and only time will tell. So I'd say this change is probably fine for now.
Would still be nic
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2515129785)
> Concept ACK, but curious for more feedback from @maflcko about this PR. The actual code changes here do not seem too complicated but maybe they make the code less generic.
There is a good chance that increasing the size of the vector is insufficient, if there is ever a need to increase it to more than 8 bytes, so a complete rewrite may be needed in that case anyway. However, this is just my guess and only time will tell. So I'd say this change is probably fine for now.
Would still be nic
...
💬 maflcko commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2515135185)
@ryanofsky Do you think this is rfm?
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2515135185)
@ryanofsky Do you think this is rfm?
💬 ryanofsky commented on pull request "rpc: Remove deprecated RPC arg names":
(https://github.com/bitcoin/bitcoin/pull/31412#issuecomment-2515136809)
re: https://github.com/bitcoin/bitcoin/pull/31412#issuecomment-2514938972
> -rpcdeprecated is limited both spatially and temporary: To only the RPC method that needs it, not the whole RPC framework, and only to a single release. So it seems preferable to me.
Got it, so it sounds like a big motivation is to simplify rpc framework code. Agree that's worth doing, and I think that should be possible even with #24963, or we could revisit this approach if #24963 doesn't go anywhere.
> re: https
...
(https://github.com/bitcoin/bitcoin/pull/31412#issuecomment-2515136809)
re: https://github.com/bitcoin/bitcoin/pull/31412#issuecomment-2514938972
> -rpcdeprecated is limited both spatially and temporary: To only the RPC method that needs it, not the whole RPC framework, and only to a single release. So it seems preferable to me.
Got it, so it sounds like a big motivation is to simplify rpc framework code. Agree that's worth doing, and I think that should be possible even with #24963, or we could revisit this approach if #24963 doesn't go anywhere.
> re: https
...
🚀 ryanofsky merged a pull request: "ci: Skip broken Wine64 tests by default"
(https://github.com/bitcoin/bitcoin/pull/31284)
(https://github.com/bitcoin/bitcoin/pull/31284)
👍 lucasbalieiro approved a pull request: "test: fix `test_invalid_tx_in_compactblock` in `p2p_compactblocks`"
(https://github.com/bitcoin/bitcoin/pull/31406#pullrequestreview-2476348912)
Tested ACK: [https://github.com/bitcoin/bitcoin/commit/9e278f7f13395219ab056ba8c2810d3ce5c41d65](https://github.com/bitcoin/bitcoin/commit/9e278f7f13395219ab056ba8c2810d3ce5c41d65)
Tested in my environment:
```bash
$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 24.04.1 LTS
Release: 24.04
Codename: noble
```
Everything ran smoothly. I understand the necessity of the change to ensure correct comparisons between integer values.
For future
...
(https://github.com/bitcoin/bitcoin/pull/31406#pullrequestreview-2476348912)
Tested ACK: [https://github.com/bitcoin/bitcoin/commit/9e278f7f13395219ab056ba8c2810d3ce5c41d65](https://github.com/bitcoin/bitcoin/commit/9e278f7f13395219ab056ba8c2810d3ce5c41d65)
Tested in my environment:
```bash
$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 24.04.1 LTS
Release: 24.04
Codename: noble
```
Everything ran smoothly. I understand the necessity of the change to ensure correct comparisons between integer values.
For future
...
💬 ryanofsky commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2515177947)
> @ryanofsky Do you think this is rfm?
Merged this now.
Am a little curious about the underlying issue though because in theory there doesn't have to be binary distinction between false positives and true positives. Test failures on a platform could indicate that be a sign that tests are fragile or poorly written even if they aren't technically buggy or reveal bugs in the code. Just clicking the cirrus links here it's hard to tell what the nature of these failures is, what may be causing t
...
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2515177947)
> @ryanofsky Do you think this is rfm?
Merged this now.
Am a little curious about the underlying issue though because in theory there doesn't have to be binary distinction between false positives and true positives. Test failures on a platform could indicate that be a sign that tests are fragile or poorly written even if they aren't technically buggy or reveal bugs in the code. Just clicking the cirrus links here it's hard to tell what the nature of these failures is, what may be causing t
...
🤔 mzumsande reviewed a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2476392187)
Code Review ACK 492e1f09943fcb6145c21d470299305a19e17d8b
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2476392187)
Code Review ACK 492e1f09943fcb6145c21d470299305a19e17d8b