💬 fanquake commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1957123270)
> My point is that it's not required on other operating systems.
Right, but it's also not required if cross-compiling to Linux from other operating systems, and yea, if you're self-compiling on Linux it doesn't really matter. This whole line could probably just be dropped.
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1957123270)
> My point is that it's not required on other operating systems.
Right, but it's also not required if cross-compiling to Linux from other operating systems, and yea, if you're self-compiling on Linux it doesn't really matter. This whole line could probably just be dropped.
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957128089)
In commit "[txorphanage] add per-peer iterator list and announcements accounting"
The cost of this `std::distance` may be *O(num_orphans_per_peer)*, and the `peer : it->second.announcers` loop around can run up to *O(num_announcers_per_tx)*. However, since the sum of all `orphan_list` lengths is equal to the total number of announcements, the overall cost of `EraseTx` is bounded by *O(total_announcements)*. In both `MaybeExpireOrphan` and `EraseForBlock`, the function `EraseTx` may be invoked
...
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957128089)
In commit "[txorphanage] add per-peer iterator list and announcements accounting"
The cost of this `std::distance` may be *O(num_orphans_per_peer)*, and the `peer : it->second.announcers` loop around can run up to *O(num_announcers_per_tx)*. However, since the sum of all `orphan_list` lengths is equal to the total number of announcements, the overall cost of `EraseTx` is bounded by *O(total_announcements)*. In both `MaybeExpireOrphan` and `EraseForBlock`, the function `EraseTx` may be invoked
...
💬 brunoerg commented on issue "Wallet fuzzing tracking issue":
(https://github.com/bitcoin/bitcoin/issues/29901#issuecomment-2660948963)
> Not sure about performance issues, but a fuzz target to cover [#31474](https://github.com/bitcoin/bitcoin/issues/31474) would be great?
I'm working on it.
(https://github.com/bitcoin/bitcoin/issues/29901#issuecomment-2660948963)
> Not sure about performance issues, but a fuzz target to cover [#31474](https://github.com/bitcoin/bitcoin/issues/31474) would be great?
I'm working on it.
💬 romanz commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2660949219)
FTR, it's possible to import the data from SQLite into DuckDB, to use its optimized query execution engine (for faster queries):
```sql
ATTACH 'utxo.sqlite' AS utxo_sqlite (TYPE SQLITE, READONLY);
ATTACH 'utxo.duckdb' AS utxo_duckdb;
COPY FROM DATABASE utxo_sqlite TO utxo_duckdb;
USE utxo_duckdb;
```
```
$ du -h utxo.*
24G utxo.sqlite
13G utxo.duckdb
```
```python
In [1]: import duckdb
In [2]: c = duckdb.connect()
In [3]: c.sql("ATTACH 'utxo.duckdb' AS utxo_duckdb")
I
...
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2660949219)
FTR, it's possible to import the data from SQLite into DuckDB, to use its optimized query execution engine (for faster queries):
```sql
ATTACH 'utxo.sqlite' AS utxo_sqlite (TYPE SQLITE, READONLY);
ATTACH 'utxo.duckdb' AS utxo_duckdb;
COPY FROM DATABASE utxo_sqlite TO utxo_duckdb;
USE utxo_duckdb;
```
```
$ du -h utxo.*
24G utxo.sqlite
13G utxo.duckdb
```
```python
In [1]: import duckdb
In [2]: c = duckdb.connect()
In [3]: c.sql("ATTACH 'utxo.duckdb' AS utxo_duckdb")
I
...
💬 laanwj commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2660957175)
Great to see that this was merged!
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2660957175)
Great to see that this was merged!
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957132917)
are you sure it's being sent via every peer for this benchmark? Looks like there's no overlap?
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957132917)
are you sure it's being sent via every peer for this benchmark? Looks like there's no overlap?
💬 willcl-ark commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2660973776)
> Ignoring the current impl details, the main question to be answered here is: Do we consider `-O3` to be harmful/dangerous?
Is it the case that the primary danger for us would be the compiler optimising out undefined behaviour? If so, then our compiler warnings and `native_asan` ci test which has the `undefined` sanitizer enabled should (in theory) catch said instances of UB?
> Currently we intercept flags for the `Release` type and downgrade to `-O2`. This would be pretty unexpected behavior
...
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2660973776)
> Ignoring the current impl details, the main question to be answered here is: Do we consider `-O3` to be harmful/dangerous?
Is it the case that the primary danger for us would be the compiler optimising out undefined behaviour? If so, then our compiler warnings and `native_asan` ci test which has the `undefined` sanitizer enabled should (in theory) catch said instances of UB?
> Currently we intercept flags for the `Release` type and downgrade to `-O2`. This would be pretty unexpected behavior
...
💬 furszy commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1957143214)
Fixed. Thx.
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1957143214)
Fixed. Thx.
💬 hodlinator commented on pull request "qa wallet: Activate dormant checks in wallet_multisig_descriptor_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/31874#issuecomment-2660984055)
Thanks for having a look @l0rinc!
> Can we use `assert_equal(all(), True)` for consistency?
I prefer `assert_true(all())`, since the condition may span the full line, so having what we are testing against at the beginning of the line is more readable. One could go Yoda-style and `assert_equal(True, all())` to solve that issue, but why not bite the bullet and support `assert_true(all())`? Other frameworks like GoogleTest [have `ASSERT_TRUE()`](https://google.github.io/googletest/reference/a
...
(https://github.com/bitcoin/bitcoin/pull/31874#issuecomment-2660984055)
Thanks for having a look @l0rinc!
> Can we use `assert_equal(all(), True)` for consistency?
I prefer `assert_true(all())`, since the condition may span the full line, so having what we are testing against at the beginning of the line is more readable. One could go Yoda-style and `assert_equal(True, all())` to solve that issue, but why not bite the bullet and support `assert_true(all())`? Other frameworks like GoogleTest [have `ASSERT_TRUE()`](https://google.github.io/googletest/reference/a
...
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957150217)
Added a few more benchmarks to get an idea of what it could look like with current PR: https://github.com/instagibbs/bitcoin/commit/ba2e3e339cafdf1b38742b2c288a18dd32c63db3
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 7,012,786.00 | 142.60 | 0.9% | 0.08 | `OrphanageEvictionBlockManyPeers`
| 27,505,341.00 | 36.36 | 0.8% |
...
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1957150217)
Added a few more benchmarks to get an idea of what it could look like with current PR: https://github.com/instagibbs/bitcoin/commit/ba2e3e339cafdf1b38742b2c288a18dd32c63db3
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 7,012,786.00 | 142.60 | 0.9% | 0.08 | `OrphanageEvictionBlockManyPeers`
| 27,505,341.00 | 36.36 | 0.8% |
...
💬 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.