💬 hodlinator commented on pull request "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/31874#issuecomment-2661058350)
> I don't really see what it adds, but won't block if others are in favor.
>
> Can we separate the two problems?
Lets see what others think. I think the PR content & size is okay as-is, both fixing a minor issue and demonstrating a slight use-case for `assert_true()`, but am open to expand it with scripted diffs if others agree. Not too keen on both expanding and splitting it at the same time though.
(https://github.com/bitcoin/bitcoin/pull/31874#issuecomment-2661058350)
> I don't really see what it adds, but won't block if others are in favor.
>
> Can we separate the two problems?
Lets see what others think. I think the PR content & size is okay as-is, both fixing a minor issue and demonstrating a slight use-case for `assert_true()`, but am open to expand it with scripted diffs if others agree. Not too keen on both expanding and splitting it at the same time though.
💬 maflcko commented on pull request "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/31874#discussion_r1957179722)
Not sure about adding a new function, because `assert` may be optimized out. Is there any report of anyone ever setting this? I'd say it is more common to not run the tests at all.
If you think this should be addressed, it would be better to check it at startup and fail early, instead of (starting to ) apply this replacement to all code.
If there was a reason to apply this to all code, it should be enforced in one way or another.
(https://github.com/bitcoin/bitcoin/pull/31874#discussion_r1957179722)
Not sure about adding a new function, because `assert` may be optimized out. Is there any report of anyone ever setting this? I'd say it is more common to not run the tests at all.
If you think this should be addressed, it would be better to check it at startup and fail early, instead of (starting to ) apply this replacement to all code.
If there was a reason to apply this to all code, it should be enforced in one way or another.
💬 hodlinator commented on pull request "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/31874#discussion_r1957181511)
I would be open to explore adding an error at startup if someone attempts executing functional tests in optimized mode so we can continue using built-in `assert` to check test conditions (several tests fail already [when attempting it](https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893728563)).
@ryanofsky expressed seeing value in reserving `assert` for internal sanity checks in the tests (see linked thread), which inspired me to author this PR in the way I did.
(https://github.com/bitcoin/bitcoin/pull/31874#discussion_r1957181511)
I would be open to explore adding an error at startup if someone attempts executing functional tests in optimized mode so we can continue using built-in `assert` to check test conditions (several tests fail already [when attempting it](https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893728563)).
@ryanofsky expressed seeing value in reserving `assert` for internal sanity checks in the tests (see linked thread), which inspired me to author this PR in the way I did.
💬 1440000bytes commented on issue "Wallet passpharse":
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2661091944)
Sorry it isn't an issue and just need to use the right format. Tried with different types of wallets (legacy, descriptor), different versions of bitcoin core on windows and linux.
bitcoin-qt:
```
walletpassphrase ' \\ \\ \\ \\ \\ \\ \\ \\ \\ \\ \\ \\' 800
```
bitcoin-cli:
```
bitcoin-cli -rpcwallet=wallet_name walletpassphrase " \\ \\ \\ \\ \\ \\ \\ \\ \\ \\ \\ \\" 800
```
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2661091944)
Sorry it isn't an issue and just need to use the right format. Tried with different types of wallets (legacy, descriptor), different versions of bitcoin core on windows and linux.
bitcoin-qt:
```
walletpassphrase ' \\ \\ \\ \\ \\ \\ \\ \\ \\ \\ \\ \\' 800
```
bitcoin-cli:
```
bitcoin-cli -rpcwallet=wallet_name walletpassphrase " \\ \\ \\ \\ \\ \\ \\ \\ \\ \\ \\ \\" 800
```
💬 adamandrews1 commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r1957200761)
@brunoerg I think you are right, I will update the description here to make it explicit we are tracking PSBT here 👍🏻
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r1957200761)
@brunoerg I think you are right, I will update the description here to make it explicit we are tracking PSBT here 👍🏻
📝 theStack opened a pull request: "doc: add release note for #27432 (utxo-to-sqlite tool)"
(https://github.com/bitcoin/bitcoin/pull/31879)
This PR adds a missing release note for https://github.com/bitcoin/bitcoin/pull/27432.
(https://github.com/bitcoin/bitcoin/pull/31879)
This PR adds a missing release note for https://github.com/bitcoin/bitcoin/pull/27432.
💬 kevkevinpal commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r1957228107)
Right now this code would not be executed at all, do you think it makes sense to add a new invalid tx with witness data to `invalid_txs.py`?
Similar to what you had [here](https://github.com/ajtowns/bitcoin/pull/13/files#diff-d390e9e12bfa34a462fdff9f1894d7dc3edb45c26cf55df486d92864a3052fafR139)
[this](https://github.com/bitcoin/bitcoin/commit/686ecf3a0c39652613635848b7efb93af2a74761) should execute `add_witness_commitment`
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r1957228107)
Right now this code would not be executed at all, do you think it makes sense to add a new invalid tx with witness data to `invalid_txs.py`?
Similar to what you had [here](https://github.com/ajtowns/bitcoin/pull/13/files#diff-d390e9e12bfa34a462fdff9f1894d7dc3edb45c26cf55df486d92864a3052fafR139)
[this](https://github.com/bitcoin/bitcoin/commit/686ecf3a0c39652613635848b7efb93af2a74761) should execute `add_witness_commitment`
💬 kevkevinpal commented on pull request "doc: add release note for #27432 (utxo-to-sqlite tool)":
(https://github.com/bitcoin/bitcoin/pull/31879#issuecomment-2661204063)
ACK [95722d0](https://github.com/bitcoin/bitcoin/pull/31879/commits/95722d048a85a313b35c69459680f734feb67695)
(https://github.com/bitcoin/bitcoin/pull/31879#issuecomment-2661204063)
ACK [95722d0](https://github.com/bitcoin/bitcoin/pull/31879/commits/95722d048a85a313b35c69459680f734feb67695)
💬 kevkevinpal commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957232376)
instead of removing this test we can keep it if we restart the node with the previous max orphan amount
```
self.restart_node(0, extra_args=["-maxorphantx=" + str(DEFAULT_MAX_ORPHAN_TRANSACTIONS)])
```
and we can probably move `DEFAULT_MAX_ORPHAN_TRANSACTIONS` into this `test_max_orphan_amount` and rename it to `max_orphan_amount` since this isnt the default max orphan amount anymore.
If we still don't want this test we can remove `DEFAULT_MAX_ORPHAN_TRANSACTIONS` since it is only use
...
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957232376)
instead of removing this test we can keep it if we restart the node with the previous max orphan amount
```
self.restart_node(0, extra_args=["-maxorphantx=" + str(DEFAULT_MAX_ORPHAN_TRANSACTIONS)])
```
and we can probably move `DEFAULT_MAX_ORPHAN_TRANSACTIONS` into this `test_max_orphan_amount` and rename it to `max_orphan_amount` since this isnt the default max orphan amount anymore.
If we still don't want this test we can remove `DEFAULT_MAX_ORPHAN_TRANSACTIONS` since it is only use
...
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1957237059)
Fixed- thanks!
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1957237059)
Fixed- thanks!
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1957237067)
Done!
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1957237067)
Done!
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1957237099)
Good point, removed example to simplify.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1957237099)
Good point, removed example to simplify.
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1957237107)
Done
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1957237107)
Done
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1957237129)
Done
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1957237129)
Done
💬 i-am-yuvi commented on pull request "test: Rename send_message to send_without_ping":
(https://github.com/bitcoin/bitcoin/pull/31859#issuecomment-2661290722)
Concept ACK
I agree!
(https://github.com/bitcoin/bitcoin/pull/31859#issuecomment-2661290722)
Concept ACK
I agree!
🤔 hebasto reviewed a pull request: "build: move `rpc/external_signer` to node library"
(https://github.com/bitcoin/bitcoin/pull/31865#pullrequestreview-2619558416)
Post-merge ACK e501246e77cc36cff222fb07aed2fd1316e11e19.
(https://github.com/bitcoin/bitcoin/pull/31865#pullrequestreview-2619558416)
Post-merge ACK e501246e77cc36cff222fb07aed2fd1316e11e19.
💬 hebasto commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2661312894)
> > I'm not sure about the `guix-build-096525e92cc2/output/dist-archive/bitcoin-096525e92cc2-codesignatures-e252afe1296a.tar.gz` artifact in the output.
>
> Not sure about it in what way?
Oh, I missed [this](https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2654397674) comment, which resolves my concerns.
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2661312894)
> > I'm not sure about the `guix-build-096525e92cc2/output/dist-archive/bitcoin-096525e92cc2-codesignatures-e252afe1296a.tar.gz` artifact in the output.
>
> Not sure about it in what way?
Oh, I missed [this](https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2654397674) comment, which resolves my concerns.
💬 hebasto commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2661323430)
> Tested a Windows-specific part:
>
> ```
> $ env HOSTS=x86_64-w64-mingw32
> $ ./contrib/guix/guix-build
> $ ./contrib/guix/guix-codesign
> $ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
> ac50a82cb146e016d5d643460dc4ff7452a70497f2d95f76cee2bcfd82724ab6 guix-build-096525e92cc2/output/dist-archive/bitcoin-096525e92cc2-codesignatures-e252afe1296a.tar.gz
> 504608f78dc2be04bda41dc212d3cbb09afd270485884b03b426ad596b4b3
...
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2661323430)
> Tested a Windows-specific part:
>
> ```
> $ env HOSTS=x86_64-w64-mingw32
> $ ./contrib/guix/guix-build
> $ ./contrib/guix/guix-codesign
> $ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
> ac50a82cb146e016d5d643460dc4ff7452a70497f2d95f76cee2bcfd82724ab6 guix-build-096525e92cc2/output/dist-archive/bitcoin-096525e92cc2-codesignatures-e252afe1296a.tar.gz
> 504608f78dc2be04bda41dc212d3cbb09afd270485884b03b426ad596b4b3
...
🤔 i-am-yuvi reviewed a pull request: "doc: add missing copyright headers"
(https://github.com/bitcoin/bitcoin/pull/31864#pullrequestreview-2619570364)
ACK 01b9a6183eb58dffac00f053e2742d924c84c721
(https://github.com/bitcoin/bitcoin/pull/31864#pullrequestreview-2619570364)
ACK 01b9a6183eb58dffac00f053e2742d924c84c721
💬 hebasto commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2661370269)
I've redone the entire process from scratch using [these](https://github.com/pinheadmz/bitcoin-detached-sigs/tree/achow101-macos-notarization-096525e92cc2) signatures:
```
207621cdc43868870f4136e9e6784a2a3e9ba89ec1edc6fa92b315cfa3c4432c guix-build-096525e92cc2/output/arm64-apple-darwin-codesigned/SHA256SUMS.part
111016205f0a2ac732feb934acb3e8a36d5251f119d8fa9215790310ba46c31d guix-build-096525e92cc2/output/arm64-apple-darwin-codesigned/bitcoin-096525e92cc2-arm64-apple-darwin.tar.gz
a1228dd
...
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2661370269)
I've redone the entire process from scratch using [these](https://github.com/pinheadmz/bitcoin-detached-sigs/tree/achow101-macos-notarization-096525e92cc2) signatures:
```
207621cdc43868870f4136e9e6784a2a3e9ba89ec1edc6fa92b315cfa3c4432c guix-build-096525e92cc2/output/arm64-apple-darwin-codesigned/SHA256SUMS.part
111016205f0a2ac732feb934acb3e8a36d5251f119d8fa9215790310ba46c31d guix-build-096525e92cc2/output/arm64-apple-darwin-codesigned/bitcoin-096525e92cc2-arm64-apple-darwin.tar.gz
a1228dd
...