💬 davidgumberg commented on issue "guix: Unable to reproduce macOS SDK tarball on Fedora 40":
(https://github.com/bitcoin/bitcoin/issues/31873#issuecomment-2660990695)
> Is the output of `tar tvvf` the same for both archives?
Diffing `tar tvvf` of the two archives results in no output.
```console
$ sha256sum badsdk/Xcode-15.0-15A240d-extracted-SDK-with-libcxx-headers.tar.gz
5b1a05d3e79fd14f5c8f6d3565762c89a522c7f5e7efbed4353d878410f2d765 badsdk/Xcode-15.0-15A240d-extracted-SDK-with-libcxx-headers.tar.gz
$ sha256sum goodsdk/Xcode-15.0-15A240d-extracted-SDK-with-libcxx-headers.tar.gz
c0c2e7bb92c1fee0c4e9f3a485e4530786732d6c6dd9e9f418c282aa6892f55d goodsdk/Xc
...
(https://github.com/bitcoin/bitcoin/issues/31873#issuecomment-2660990695)
> Is the output of `tar tvvf` the same for both archives?
Diffing `tar tvvf` of the two archives results in no output.
```console
$ sha256sum badsdk/Xcode-15.0-15A240d-extracted-SDK-with-libcxx-headers.tar.gz
5b1a05d3e79fd14f5c8f6d3565762c89a522c7f5e7efbed4353d878410f2d765 badsdk/Xcode-15.0-15A240d-extracted-SDK-with-libcxx-headers.tar.gz
$ sha256sum goodsdk/Xcode-15.0-15A240d-extracted-SDK-with-libcxx-headers.tar.gz
c0c2e7bb92c1fee0c4e9f3a485e4530786732d6c6dd9e9f418c282aa6892f55d goodsdk/Xc
...
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957152330)
@instagibbs I believe the worst case when max_orphans == max_announcements is to have exactly one peer, and then erasing the transactions in reverse order they appear in _m_list_iters.
That would cost n^2/2 steps in std::distance.
When max_announcements is larger than max_orphans, the worst case is having max_announcements / max_orphans peers, and every transaction be announced by all, I think.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957152330)
@instagibbs I believe the worst case when max_orphans == max_announcements is to have exactly one peer, and then erasing the transactions in reverse order they appear in _m_list_iters.
That would cost n^2/2 steps in std::distance.
When max_announcements is larger than max_orphans, the worst case is having max_announcements / max_orphans peers, and every transaction be announced by all, I think.
💬 Sjors commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2660996045)
macOS intel is happy too, no more right-clickery.
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2660996045)
macOS intel is happy too, no more right-clickery.
💬 l0rinc commented on pull request "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/31874#issuecomment-2660998528)
> why not bite the bullet and support
I don't really see what it adds, but won't block if others are in favor.
Can we separate the two problems?
I mean adding back the assertions in one PR (though I'd vote for fixing all remaining `assert (all|any` in the same PR that fixes the unasserted ones), and proposing the new `assert_true` together with a scripted diff that applies it, to see if reviewers (or at least the author) thinks that `assert_true` is indeed better than `assert_equals(True,
...
(https://github.com/bitcoin/bitcoin/pull/31874#issuecomment-2660998528)
> why not bite the bullet and support
I don't really see what it adds, but won't block if others are in favor.
Can we separate the two problems?
I mean adding back the assertions in one PR (though I'd vote for fixing all remaining `assert (all|any` in the same PR that fixes the unasserted ones), and proposing the new `assert_true` together with a scripted diff that applies it, to see if reviewers (or at least the author) thinks that `assert_true` is indeed better than `assert_equals(True,
...
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957154102)
> I believe the worst case when max_orphans == max_announcements is to have exactly one peer, and then erasing the transactions in reverse order they appear in _m_list_iters.
I swapped the order of the block txns to force it to walk the whole list for the single peer, it's about 10% slower
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957154102)
> I believe the worst case when max_orphans == max_announcements is to have exactly one peer, and then erasing the transactions in reverse order they appear in _m_list_iters.
I swapped the order of the block txns to force it to walk the whole list for the single peer, it's about 10% slower
👍 rkrux approved a pull request: "wallet: abandon orphan coinbase txs, and their descendants, during startup"
(https://github.com/bitcoin/bitcoin/pull/31794#pullrequestreview-2619470433)
tACK e4dd5a351bde88a94326824945f4c8b1e4c15df2
(https://github.com/bitcoin/bitcoin/pull/31794#pullrequestreview-2619470433)
tACK e4dd5a351bde88a94326824945f4c8b1e4c15df2
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957169813)
Another solution maybe: replace `set<NodeId> announcers` in `OrphanTxBase` with a `std::map<NodeId, size_t> announcers`, where the value is the orphan's position in the `PeerOrphanInfo::m_iter_list`. Previously, we had a `list_pos`that tracked the orphan's location in `m_orphan_list`. That removes the need to do `std::distance`.
On the whole though, I agree a multiindex is probably the most natural data structure for orphanage.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957169813)
Another solution maybe: replace `set<NodeId> announcers` in `OrphanTxBase` with a `std::map<NodeId, size_t> announcers`, where the value is the orphan's position in the `PeerOrphanInfo::m_iter_list`. Previously, we had a `list_pos`that tracked the orphan's location in `m_orphan_list`. That removes the need to do `std::distance`.
On the whole though, I agree a multiindex is probably the most natural data structure for orphanage.
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957171207)
@glozow That works, I believe.
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957171207)
@glozow That works, I believe.
💬 1440000bytes commented on issue "Wallet passpharse":
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2661044983)
I am able to reproduce the issue on Windows with this password ` \ \ \ \ \ \ \ \ \ \ \ \`. I will need some time to share more information.
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2661044983)
I am able to reproduce the issue on Windows with this password ` \ \ \ \ \ \ \ \ \ \ \ \`. I will need some time to share more information.
💬 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!