💬 kevkevinpal commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2077905380)
Yes that makes sense to me!
updated in [ed4aef6](https://github.com/bitcoin/bitcoin/pull/32243/commits/ed4aef699801d79fb34211282007c87e4451b604)
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2077905380)
Yes that makes sense to me!
updated in [ed4aef6](https://github.com/bitcoin/bitcoin/pull/32243/commits/ed4aef699801d79fb34211282007c87e4451b604)
💬 kevkevinpal commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#issuecomment-2859026107)
> Good stuff, nice to have new coverage.
>
> I think it'd be worth it to add an oracle in the test where you reconstruct the merkle root with the path you get from `TransactionMerklePath` and verify that it matches the root from the earlier `ComputeMerkleRoot` call.
Thank you for the review!
In [ed4aef6](https://github.com/bitcoin/bitcoin/pull/32243/commits/ed4aef699801d79fb34211282007c87e4451b604) I added a helper function ComputeMerkleRootFromPath to help rebuild the root so then I co
...
(https://github.com/bitcoin/bitcoin/pull/32243#issuecomment-2859026107)
> Good stuff, nice to have new coverage.
>
> I think it'd be worth it to add an oracle in the test where you reconstruct the merkle root with the path you get from `TransactionMerklePath` and verify that it matches the root from the earlier `ComputeMerkleRoot` call.
Thank you for the review!
In [ed4aef6](https://github.com/bitcoin/bitcoin/pull/32243/commits/ed4aef699801d79fb34211282007c87e4451b604) I added a helper function ComputeMerkleRootFromPath to help rebuild the root so then I co
...
💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2859036255)
Rebased after https://github.com/bitcoin/bitcoin/pull/28710
(https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2859036255)
Rebased after https://github.com/bitcoin/bitcoin/pull/28710
💬 fanquake commented on pull request "crypto: disable ASan for sha256_sse4 with Clang":
(https://github.com/bitcoin/bitcoin/pull/32437#issuecomment-2859044849)
Guix build:
```bash
20608607794819e22c0bc02c920ba03a4edf98b902e278f1e2dcc9d92f2f485f guix-build-4e8ab5e00fa7/output/aarch64-linux-gnu/SHA256SUMS.part
f3268558a957a2b55191b14bdb9f7c23d60dc72a84ecbfe6b56a280f8f09fd1f guix-build-4e8ab5e00fa7/output/aarch64-linux-gnu/bitcoin-4e8ab5e00fa7-aarch64-linux-gnu-debug.tar.gz
791e1b3e81b358343dd5fafa53ab1d503e819d6935515277443dcfef4091c470 guix-build-4e8ab5e00fa7/output/aarch64-linux-gnu/bitcoin-4e8ab5e00fa7-aarch64-linux-gnu.tar.gz
1b689d1c92f6026d
...
(https://github.com/bitcoin/bitcoin/pull/32437#issuecomment-2859044849)
Guix build:
```bash
20608607794819e22c0bc02c920ba03a4edf98b902e278f1e2dcc9d92f2f485f guix-build-4e8ab5e00fa7/output/aarch64-linux-gnu/SHA256SUMS.part
f3268558a957a2b55191b14bdb9f7c23d60dc72a84ecbfe6b56a280f8f09fd1f guix-build-4e8ab5e00fa7/output/aarch64-linux-gnu/bitcoin-4e8ab5e00fa7-aarch64-linux-gnu-debug.tar.gz
791e1b3e81b358343dd5fafa53ab1d503e819d6935515277443dcfef4091c470 guix-build-4e8ab5e00fa7/output/aarch64-linux-gnu/bitcoin-4e8ab5e00fa7-aarch64-linux-gnu.tar.gz
1b689d1c92f6026d
...
📝 maflcko opened a pull request: "refactor: Removals after bdb removal"
(https://github.com/bitcoin/bitcoin/pull/32438)
This deletes some dead code
(https://github.com/bitcoin/bitcoin/pull/32438)
This deletes some dead code
💬 maflcko commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2859058100)
follow-up in https://github.com/bitcoin/bitcoin/pull/32438
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2859058100)
follow-up in https://github.com/bitcoin/bitcoin/pull/32438
💬 jesterhodl commented on pull request "qt, docs: Unify term "clipboard"":
(https://github.com/bitcoin-core/gui/pull/871#issuecomment-2859064246)
Would be good to do the same for all remaining *.ts files in ```src/qt/locale/``` as well as these two:
`
src/qt/forms/signverifymessagedialog.ui: <string>Copy the current signature to the system clipboard</string>
src/qt/forms/addressbookpage.ui: <string>Copy the currently selected address to the system clipboard</string>
`
I used ```grep -ri "system clipboard" *``` to find them
(https://github.com/bitcoin-core/gui/pull/871#issuecomment-2859064246)
Would be good to do the same for all remaining *.ts files in ```src/qt/locale/``` as well as these two:
`
src/qt/forms/signverifymessagedialog.ui: <string>Copy the current signature to the system clipboard</string>
src/qt/forms/addressbookpage.ui: <string>Copy the currently selected address to the system clipboard</string>
`
I used ```grep -ri "system clipboard" *``` to find them
💬 joanrodriguez commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2859080055)
Is it true that @petertodd got paid to open this PR? And who is Christopher Allen?
What problem does this solve?
Thanks
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2859080055)
Is it true that @petertodd got paid to open this PR? And who is Christopher Allen?
What problem does this solve?
Thanks
💬 hebasto commented on pull request "depends: Switch from multilib to platform-specific toolchains":
(https://github.com/bitcoin/bitcoin/pull/32162#issuecomment-2859081895)
My Guix build:
```
aarch64
44c90cd188bf6f78d06831c61d74b141cdefb12d2a786b4d65235b310862ccff guix-build-5f061f5abdee/output/aarch64-linux-gnu/SHA256SUMS.part
347230a10b4f1a169442f41ff6ebdd09565d9b6239e8894e653fd08f16d6f9ca guix-build-5f061f5abdee/output/aarch64-linux-gnu/bitcoin-5f061f5abdee-aarch64-linux-gnu-debug.tar.gz
cee271b441733749e6070ece4b14dd2090d3a273ebc86d5331baf8c991bdd8fe guix-build-5f061f5abdee/output/aarch64-linux-gnu/bitcoin-5f061f5abdee-aarch64-linux-gnu.tar.gz
9f7c9748
...
(https://github.com/bitcoin/bitcoin/pull/32162#issuecomment-2859081895)
My Guix build:
```
aarch64
44c90cd188bf6f78d06831c61d74b141cdefb12d2a786b4d65235b310862ccff guix-build-5f061f5abdee/output/aarch64-linux-gnu/SHA256SUMS.part
347230a10b4f1a169442f41ff6ebdd09565d9b6239e8894e653fd08f16d6f9ca guix-build-5f061f5abdee/output/aarch64-linux-gnu/bitcoin-5f061f5abdee-aarch64-linux-gnu-debug.tar.gz
cee271b441733749e6070ece4b14dd2090d3a273ebc86d5331baf8c991bdd8fe guix-build-5f061f5abdee/output/aarch64-linux-gnu/bitcoin-5f061f5abdee-aarch64-linux-gnu.tar.gz
9f7c9748
...
👍 Sjors approved a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2822182388)
ACK 8c360d78b13fa7136db8d6e1e0af5d05f5141a64
The GUI popup can be fixed in a followup (before v30).
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2822182388)
ACK 8c360d78b13fa7136db8d6e1e0af5d05f5141a64
The GUI popup can be fixed in a followup (before v30).
💬 Sjors commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077873264)
f4ae3126ea9650c4da9aa80c0f8c5d12da84be2f: in the GUI this results in a popup every time you start the node.
`m_warnings` could be used instead to treat it similar to the "This is a pre-release test build" message. Or we'd need a way to permanently dismiss a given warning string, which would be a GUI followup.
For similar discussion, see https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2685149599
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077873264)
f4ae3126ea9650c4da9aa80c0f8c5d12da84be2f: in the GUI this results in a popup every time you start the node.
`m_warnings` could be used instead to treat it similar to the "This is a pre-release test build" message. Or we'd need a way to permanently dismiss a given warning string, which would be a GUI followup.
For similar discussion, see https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2685149599
💬 Sjors commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077900934)
062fbe7f894136ee1f9d52c62d42bdf7b2cc1c0a: could comment here that both `MAX_SCRIPT_ELEMENT_SIZE` (520) and `MAX_SCRIPT_SIZE` (10,000) are only evaluated when spending an output, not when creating one. This is why 10K bytes fit here despite the 4 byte overhead from `OP_RETURN`, `OP_PUSHDATA2` and two length bytes.
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077900934)
062fbe7f894136ee1f9d52c62d42bdf7b2cc1c0a: could comment here that both `MAX_SCRIPT_ELEMENT_SIZE` (520) and `MAX_SCRIPT_SIZE` (10,000) are only evaluated when spending an output, not when creating one. This is why 10K bytes fit here despite the 4 byte overhead from `OP_RETURN`, `OP_PUSHDATA2` and two length bytes.
💬 Sjors commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077932108)
4426025fef23b014da8b8caab64239ba8f46a297: although this more specific error message is an improvement, there might be software out there that expects `scriptpubkey`. If so, it would be pointless for them to change their code for something that we're deprecating anyway.
I asked co-pilot to look:
---
The string "multi-op-return" is used in multiple repositories, primarily to check for transactions containing multiple `OP_RETURN` outputs, which are considered non-standard by Bitcoin Core p
...
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077932108)
4426025fef23b014da8b8caab64239ba8f46a297: although this more specific error message is an improvement, there might be software out there that expects `scriptpubkey`. If so, it would be pointless for them to change their code for something that we're deprecating anyway.
I asked co-pilot to look:
---
The string "multi-op-return" is used in multiple repositories, primarily to check for transactions containing multiple `OP_RETURN` outputs, which are considered non-standard by Bitcoin Core p
...
💬 jesterhodl commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2859103803)
> Concept ACK
>
> The message Satoshi Nakamoto inscribed into the Genesis Block is 69 bytes, yet many advocate for reducing it to ~40. This change would have prevented Bitcoin's emergent criticism of the existing fiat system from being included. Satoshi stored "arbitrary data" in the very first block. To say that such usage is "spam" is to ignore Bitcoin's origin.
>
> The world's most neutral and immutable ledger should be unopinionated towards what the free market would like, and pays for
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2859103803)
> Concept ACK
>
> The message Satoshi Nakamoto inscribed into the Genesis Block is 69 bytes, yet many advocate for reducing it to ~40. This change would have prevented Bitcoin's emergent criticism of the existing fiat system from being included. Satoshi stored "arbitrary data" in the very first block. To say that such usage is "spam" is to ignore Bitcoin's origin.
>
> The world's most neutral and immutable ledger should be unopinionated towards what the free market would like, and pays for
...
💬 hebasto commented on pull request "qt, docs: Unify term "clipboard"":
(https://github.com/bitcoin-core/gui/pull/871#issuecomment-2859111862)
> Would be good to do the same for all remaining *.ts files in `src/qt/locale/` as well as these two: `src/qt/forms/signverifymessagedialog.ui: <string>Copy the current signature to the system clipboard</string> src/qt/forms/addressbookpage.ui: <string>Copy the currently selected address to the system clipboard</string>`
>
> I used `grep -ri "system clipboard" *` to find them
The `src/qt/locale/` directory is populated during the release process by fetching translations from Transifex. Se
...
(https://github.com/bitcoin-core/gui/pull/871#issuecomment-2859111862)
> Would be good to do the same for all remaining *.ts files in `src/qt/locale/` as well as these two: `src/qt/forms/signverifymessagedialog.ui: <string>Copy the current signature to the system clipboard</string> src/qt/forms/addressbookpage.ui: <string>Copy the currently selected address to the system clipboard</string>`
>
> I used `grep -ri "system clipboard" *` to find them
The `src/qt/locale/` directory is populated during the release process by fetching translations from Transifex. Se
...
💬 Sjors commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077963070)
Make is an alternative node implementation, so it doesn't _use_ Bitcoin Core.
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2077963070)
Make is an alternative node implementation, so it doesn't _use_ Bitcoin Core.
💬 maflcko commented on pull request "refactor: Removals after bdb removal":
(https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2859123413)
There are still some potential follow-ups for someone else:
```
git grep -i 'legacy wallet' src/bitcoin-wallet.cpp src/wallet src/script/
```
As well as `assert_is_bdb` in `test/functional/tool_wallet.py`.
Also, (`labelWatch*` and `fHaveWatch*`) for the gui people.
(https://github.com/bitcoin/bitcoin/pull/32438#issuecomment-2859123413)
There are still some potential follow-ups for someone else:
```
git grep -i 'legacy wallet' src/bitcoin-wallet.cpp src/wallet src/script/
```
As well as `assert_is_bdb` in `test/functional/tool_wallet.py`.
Also, (`labelWatch*` and `fHaveWatch*`) for the gui people.
💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2859142394)
My Guix build:
```
aarch64
6b1411a20a83a7c8b791ff9ff738748b92d1a86fc7ac2337257ce418e0b966ca guix-build-3a18075aedd7/output/dist-archive/bitcoin-3a18075aedd7.tar.gz
e2f572ad6ac20ed0b66c2db7a81997cf9e090c695364d88fda1c183efe2a2cbd guix-build-3a18075aedd7/output/x86_64-w64-mingw32/SHA256SUMS.part
886647c8c0322b1a450cc97efc1bf6a41b5efc8bf02477bb22a694cea1314a42 guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-codesigning.tar.gz
4a82a01d9a12fee247e33973480183bf8a2
...
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2859142394)
My Guix build:
```
aarch64
6b1411a20a83a7c8b791ff9ff738748b92d1a86fc7ac2337257ce418e0b966ca guix-build-3a18075aedd7/output/dist-archive/bitcoin-3a18075aedd7.tar.gz
e2f572ad6ac20ed0b66c2db7a81997cf9e090c695364d88fda1c183efe2a2cbd guix-build-3a18075aedd7/output/x86_64-w64-mingw32/SHA256SUMS.part
886647c8c0322b1a450cc97efc1bf6a41b5efc8bf02477bb22a694cea1314a42 guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-codesigning.tar.gz
4a82a01d9a12fee247e33973480183bf8a2
...
💬 hebasto commented on pull request "depends: Avoid using helper variables in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31360#issuecomment-2859169316)
Rebased due to a conflict with the merged bitcoin/bitcoin#28710.
(https://github.com/bitcoin/bitcoin/pull/31360#issuecomment-2859169316)
Rebased due to a conflict with the merged bitcoin/bitcoin#28710.
💬 davidhrinaldo commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2859202220)
Concept NACK
As a node runner I am fine with the default `-datacarrier` and `-datacarriersize` as is. I am also fine with having the option of _disabling_ OP_RETURN limits if the user desires. I am _not_ okay with the software taking away my ability to decide for myself. If this is merged I will seek alternative software for my node.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2859202220)
Concept NACK
As a node runner I am fine with the default `-datacarrier` and `-datacarriersize` as is. I am also fine with having the option of _disabling_ OP_RETURN limits if the user desires. I am _not_ okay with the software taking away my ability to decide for myself. If this is merged I will seek alternative software for my node.