💬 eval-exec commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1957015376)
Squashed.
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1957015376)
Squashed.
⚠️ kousay311 opened an issue: "3"
(https://github.com/bitcoin/bitcoin/issues/31876)
### Motivation
// Good: UpperCamelCase standalone function name
std::unique_ptr<Node> MakeNode(LocalInit& init);
// Bad: lowercase standalone function
std::unique_ptr<Node> makeNode(LocalInit& init);
### Possible solution
[](url)// Good: UpperCamelCase standalone function name
std::unique_ptr<Node> MakeNode(LocalInit& init);
// Bad: lowercase standalone function
std::unique_ptr<Node> makeNode(LocalInit& init);
// Good: lowerCamelCase method name
virtual void blockConnected(const CBlock& blo
...
(https://github.com/bitcoin/bitcoin/issues/31876)
### Motivation
// Good: UpperCamelCase standalone function name
std::unique_ptr<Node> MakeNode(LocalInit& init);
// Bad: lowercase standalone function
std::unique_ptr<Node> makeNode(LocalInit& init);
### Possible solution
[](url)// Good: UpperCamelCase standalone function name
std::unique_ptr<Node> MakeNode(LocalInit& init);
// Bad: lowercase standalone function
std::unique_ptr<Node> makeNode(LocalInit& init);
// Good: lowerCamelCase method name
virtual void blockConnected(const CBlock& blo
...
💬 TheCharlatan commented on issue "guix: Unable to reproduce macOS SDK tarball on Fedora 40":
(https://github.com/bitcoin/bitcoin/issues/31873#issuecomment-2660780429)
Is the output of `tar tvvf` the same for both archives?
(https://github.com/bitcoin/bitcoin/issues/31873#issuecomment-2660780429)
Is the output of `tar tvvf` the same for both archives?
🤔 rkrux reviewed a pull request: "wallet: abandon orphan coinbase txs, and their descendants, during startup"
(https://github.com/bitcoin/bitcoin/pull/31794#pullrequestreview-2619349860)
```
git range-diff 409241db5dca0b23f5c7714f99be52411fc5541e...3c89cab06a6259fa70eaf3a78c01ee42780c4e27
```
Newer changes are updating the functional tests based on the comments suggested earlier. I will ACK once a small bug is corrected.
(https://github.com/bitcoin/bitcoin/pull/31794#pullrequestreview-2619349860)
```
git range-diff 409241db5dca0b23f5c7714f99be52411fc5541e...3c89cab06a6259fa70eaf3a78c01ee42780c4e27
```
Newer changes are updating the functional tests based on the comments suggested earlier. I will ACK once a small bug is corrected.
💬 rkrux commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1957061812)
Hmm fair enough.
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1957061812)
Hmm fair enough.
💬 rkrux commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1957063827)
Need to use `:2`, otherwise it just syncs 1 block and iterates over 1 block - the first one.
```python
self.sync_blocks(self.nodes[:2])
self.disconnect_nodes(1, 0)
assert all(len(node.getpeerinfo()) == 0 for node in self.nodes[:2])
```
(https://github.com/bitcoin/bitcoin/pull/31794#discussion_r1957063827)
Need to use `:2`, otherwise it just syncs 1 block and iterates over 1 block - the first one.
```python
self.sync_blocks(self.nodes[:2])
self.disconnect_nodes(1, 0)
assert all(len(node.getpeerinfo()) == 0 for node in self.nodes[:2])
```
✅ fanquake closed an issue: "3"
(https://github.com/bitcoin/bitcoin/issues/31876)
(https://github.com/bitcoin/bitcoin/issues/31876)
💬 theStack commented on pull request "rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script":
(https://github.com/bitcoin/bitcoin/pull/31560#issuecomment-2660845212)
Rebased on master (now that #27432 has been merged 🎉).
(https://github.com/bitcoin/bitcoin/pull/31560#issuecomment-2660845212)
Rebased on master (now that #27432 has been merged 🎉).
💬 fjahr commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2660890740)
post-merge tACK 4080b66cbec2b6fc2fcfd7356941236f65d508e3
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2660890740)
post-merge tACK 4080b66cbec2b6fc2fcfd7356941236f65d508e3
⚠️ 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% |
...