⚠️ Davidson-Souza opened an issue: "kernel: feedback on using kernel in alternative implementations"
(https://github.com/bitcoin/bitcoin/issues/31878)
After talking about this with many core devs offline, and being told every time to open an issue with this, I'm opening this to spark some discussion about the viability and optimal implementation of a `libbitcoinkernel`-like lib to help consensus-compatible implementations. This could be useful for other experimental projects like [2][3].
## Goals
Bitcoin doesn’t have a formal spec for its base protocol. It operates with the “reference implementation” model, where Bitcoin
Core dictates what B
...
(https://github.com/bitcoin/bitcoin/issues/31878)
After talking about this with many core devs offline, and being told every time to open an issue with this, I'm opening this to spark some discussion about the viability and optimal implementation of a `libbitcoinkernel`-like lib to help consensus-compatible implementations. This could be useful for other experimental projects like [2][3].
## Goals
Bitcoin doesn’t have a formal spec for its base protocol. It operates with the “reference implementation” model, where Bitcoin
Core dictates what B
...
💬 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.