💬 pinheadmz commented on pull request "Remove unnecessary casts when calling socket operations":
(https://github.com/bitcoin/bitcoin/pull/33378#issuecomment-3298628111)
Thanks for the edit @fanquake 🥰
(https://github.com/bitcoin/bitcoin/pull/33378#issuecomment-3298628111)
Thanks for the edit @fanquake 🥰
👍 vasild approved a pull request: "Remove unnecessary casts when calling socket operations"
(https://github.com/bitcoin/bitcoin/pull/33378#pullrequestreview-3229896344)
ACK 67f632b6deb8b4aa190c458b71d2bc8c793626d5
(https://github.com/bitcoin/bitcoin/pull/33378#pullrequestreview-3229896344)
ACK 67f632b6deb8b4aa190c458b71d2bc8c793626d5
📝 willcl-ark opened a pull request: "WIP: Backport Cirrus runners to 28.x"
(https://github.com/bitcoin/bitcoin/pull/33406)
Backports https://github.com/bitcoin/bitcoin/pull/32989 to the 28.x branch
(https://github.com/bitcoin/bitcoin/pull/33406)
Backports https://github.com/bitcoin/bitcoin/pull/32989 to the 28.x branch
💬 HowHsu commented on pull request "help: enrich help text for `-loadblock`":
(https://github.com/bitcoin/bitcoin/pull/33343#issuecomment-3298744240)
> @HowHsu Did you try `contrib/linearize` yourself to export the `.blk` files for `-loadblock`? Did it solve your issue? I have suggested a simpler documentation, do you think that help others to use the linearize tool when they're in your situation?
I didn't, but I read the code of them (files under `contrib/linearize`), it is not for arbitrary blocks, it only accepts a list of blocks with contiguous heights. So I just add `for clues of deobfuscation and reobfuscation`. For my scenario, I'v
...
(https://github.com/bitcoin/bitcoin/pull/33343#issuecomment-3298744240)
> @HowHsu Did you try `contrib/linearize` yourself to export the `.blk` files for `-loadblock`? Did it solve your issue? I have suggested a simpler documentation, do you think that help others to use the linearize tool when they're in your situation?
I didn't, but I read the code of them (files under `contrib/linearize`), it is not for arbitrary blocks, it only accepts a list of blocks with contiguous heights. So I just add `for clues of deobfuscation and reobfuscation`. For my scenario, I'v
...
📝 hebasto opened a pull request: "cmake: Install `bitcoin` manpage"
(https://github.com/bitcoin/bitcoin/pull/33407)
This PR is an amendment to https://github.com/bitcoin/bitcoin/pull/33348.
(https://github.com/bitcoin/bitcoin/pull/33407)
This PR is an amendment to https://github.com/bitcoin/bitcoin/pull/33348.
💬 fjahr commented on issue "Default ASmap file path is not used unless -asmap is set":
(https://github.com/bitcoin/bitcoin/issues/33386#issuecomment-3298756998)
I agree that this whole default location thing is a bit odd and it's not something I think we necessarily need but it was already there when I started to get more involved in ASmap and I didn't find it so harmful that it needed to be removed. It can be easily ignored by just not putting a file there after all, so if you don't like it just don't use it, I guess.
> The -asmap option supposedly has a default value ip_asn.map (as defined by DEFAULT_ASMAP_FILENAME). However, when I tried to put a fi
...
(https://github.com/bitcoin/bitcoin/issues/33386#issuecomment-3298756998)
I agree that this whole default location thing is a bit odd and it's not something I think we necessarily need but it was already there when I started to get more involved in ASmap and I didn't find it so harmful that it needed to be removed. It can be easily ignored by just not putting a file there after all, so if you don't like it just don't use it, I guess.
> The -asmap option supposedly has a default value ip_asn.map (as defined by DEFAULT_ASMAP_FILENAME). However, when I tried to put a fi
...
💬 fanquake commented on pull request "cmake: Install `bitcoin` manpage":
(https://github.com/bitcoin/bitcoin/pull/33407#issuecomment-3298757665)
> This PR is an amendment to https://github.com/bitcoin/bitcoin/pull/33348.
Not sure this is related to #33348. It should have been part of #31375?
(https://github.com/bitcoin/bitcoin/pull/33407#issuecomment-3298757665)
> This PR is an amendment to https://github.com/bitcoin/bitcoin/pull/33348.
Not sure this is related to #33348. It should have been part of #31375?
💬 hebasto commented on pull request "cmake: Install `bitcoin` manpage":
(https://github.com/bitcoin/bitcoin/pull/33407#issuecomment-3298763878)
> > This PR is an amendment to #33348.
>
> Not sure this is related to #33348. It should have been part of #31375?
The PR description has been updated.
(https://github.com/bitcoin/bitcoin/pull/33407#issuecomment-3298763878)
> > This PR is an amendment to #33348.
>
> Not sure this is related to #33348. It should have been part of #31375?
The PR description has been updated.
💬 fanquake commented on pull request "WIP: Backport Cirrus runners to 29.x":
(https://github.com/bitcoin/bitcoin/pull/33403#issuecomment-3298778883)
Looking at the bottom of https://github.com/bitcoin/bitcoin/actions/runs/17765745116?pr=33403, I see
```bash
x86_64-w64-mingw32-executables-17765745116 ... 4.12 KB
```
Looking in the tarball, all the executables are 200 bytes of shell script:
```bash
cat x86_64-w64-mingw32-executables-17765745116/bin/bench_bitcoin.exe
#!/usr/bin/env bash
( wine "/home/admin/actions-runner/_work/_temp/build/bin/bench_bitcoin.exe_orig" "$@" ) || ( sleep 1 && wine "/home/admin/actions-runner/_wo
...
(https://github.com/bitcoin/bitcoin/pull/33403#issuecomment-3298778883)
Looking at the bottom of https://github.com/bitcoin/bitcoin/actions/runs/17765745116?pr=33403, I see
```bash
x86_64-w64-mingw32-executables-17765745116 ... 4.12 KB
```
Looking in the tarball, all the executables are 200 bytes of shell script:
```bash
cat x86_64-w64-mingw32-executables-17765745116/bin/bench_bitcoin.exe
#!/usr/bin/env bash
( wine "/home/admin/actions-runner/_work/_temp/build/bin/bench_bitcoin.exe_orig" "$@" ) || ( sleep 1 && wine "/home/admin/actions-runner/_wo
...
💬 hebasto commented on issue "oss-fuzz: build failing":
(https://github.com/bitcoin/bitcoin/issues/33366#issuecomment-3298802545)
> [google/oss-fuzz#13987](https://github.com/google/oss-fuzz/pull/13987)
... or github.com/google/oss-fuzz/pull/14015.
(https://github.com/bitcoin/bitcoin/issues/33366#issuecomment-3298802545)
> [google/oss-fuzz#13987](https://github.com/google/oss-fuzz/pull/13987)
... or github.com/google/oss-fuzz/pull/14015.
📝 Sjors converted_to_draft a pull request: "mining: add applySolution() to interface"
(https://github.com/bitcoin/bitcoin/pull/33374)
Unlike the `submitblock` RPC which takes a fully serialized block, when a block solution is received via IPC the client only provides the nonce, coinbase and a few other fields. It may not have all the information it needs to reconstruct the block. This makes debugging difficult when the block is invalid.
This commit adds `applySolution()` which returns the reconstructed block and can be used by the client for debugging. It's assigned `@11` in the `.cap` file, and will not break existing clie
...
(https://github.com/bitcoin/bitcoin/pull/33374)
Unlike the `submitblock` RPC which takes a fully serialized block, when a block solution is received via IPC the client only provides the nonce, coinbase and a few other fields. It may not have all the information it needs to reconstruct the block. This makes debugging difficult when the block is invalid.
This commit adds `applySolution()` which returns the reconstructed block and can be used by the client for debugging. It's assigned `@11` in the `.cap` file, and will not break existing clie
...
💬 hMsats commented on issue "bitcoind 29.0 much slower than 28.0 on my system: cause found":
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-3298839070)
@martinatime An I/O issue is consistent with what I found and wrote above (could have used `-blocksdir` and ` -datadir`):
> The -reindex has finished but I had to put the chainstate and indexes directories on the internal disk of the laptop I
> used because the sync became painfully slow after a while with those directories on the external SSD (USB 3.0).
> After putting those directories on the internal SSD, syncing became 16 times faster!
6 days is acceptable for a RPi with an USB SSD
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-3298839070)
@martinatime An I/O issue is consistent with what I found and wrote above (could have used `-blocksdir` and ` -datadir`):
> The -reindex has finished but I had to put the chainstate and indexes directories on the internal disk of the laptop I
> used because the sync became painfully slow after a while with those directories on the external SSD (USB 3.0).
> After putting those directories on the internal SSD, syncing became 16 times faster!
6 days is acceptable for a RPi with an USB SSD
💬 purpleKarrot commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3298876285)
Code review ACK. I very much welcome the reduced complexity in the CMake configuration. Thanks for working on this!
I haven't tested actually running the scripts yet.
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3298876285)
Code review ACK. I very much welcome the reduced complexity in the CMake configuration. Thanks for working on this!
I haven't tested actually running the scripts yet.
💬 willcl-ark commented on pull request "WIP: Backport Cirrus runners to 29.x":
(https://github.com/bitcoin/bitcoin/pull/33403#issuecomment-3298895762)
> Looking at the bottom of [bitcoin/bitcoin/actions/runs/17765745116?pr=33403](https://github.com/bitcoin/bitcoin/actions/runs/17765745116?pr=33403), I see
>
> ```shell
> x86_64-w64-mingw32-executables-17765745116 ... 4.12 KB
> ```
>
> Looking in the tarball, all the executables are 200 bytes of shell script:
>
> ```shell
> cat x86_64-w64-mingw32-executables-17765745116/bin/bench_bitcoin.exe
> #!/usr/bin/env bash
> ( wine "/home/admin/actions-runner/_work/_temp/build/bi
...
(https://github.com/bitcoin/bitcoin/pull/33403#issuecomment-3298895762)
> Looking at the bottom of [bitcoin/bitcoin/actions/runs/17765745116?pr=33403](https://github.com/bitcoin/bitcoin/actions/runs/17765745116?pr=33403), I see
>
> ```shell
> x86_64-w64-mingw32-executables-17765745116 ... 4.12 KB
> ```
>
> Looking in the tarball, all the executables are 200 bytes of shell script:
>
> ```shell
> cat x86_64-w64-mingw32-executables-17765745116/bin/bench_bitcoin.exe
> #!/usr/bin/env bash
> ( wine "/home/admin/actions-runner/_work/_temp/build/bi
...
🤔 rkrux reviewed a pull request: "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys"
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3230143685)
Reviewed commit 7c085554dce336eb1597ab2fc482163876a49270 with the aim of deduplicating MuSig2 signing flow.
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3230143685)
Reviewed commit 7c085554dce336eb1597ab2fc482163876a49270 with the aim of deduplicating MuSig2 signing flow.
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2352608586)
In https://github.com/bitcoin/bitcoin/commit/7c085554dce336eb1597ab2fc482163876a49270 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"
I have noticed there is lot of duplication in MuSig2 signing flows done in the key path and script path spending. A common `SignMuSig2` function can help in removing duplication and risks of introducing discrepancies in the two spending paths - I checked that the tests pass. Also, should make the review easier by reducing diff.
<details open
...
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2352608586)
In https://github.com/bitcoin/bitcoin/commit/7c085554dce336eb1597ab2fc482163876a49270 "sign: Create MuSig2 signatures for known MuSig2 aggregate keys"
I have noticed there is lot of duplication in MuSig2 signing flows done in the key path and script path spending. A common `SignMuSig2` function can help in removing duplication and risks of introducing discrepancies in the two spending paths - I checked that the tests pass. Also, should make the review easier by reducing diff.
<details open
...
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2352617109)
This^ diff also uses the function from this earlier suggestion https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2336962962.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2352617109)
This^ diff also uses the function from this earlier suggestion https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2336962962.
💬 ismaelsadeeq commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2352347809)
In "txgraph: Make level of Cluster implicit (optimization)" 22f5fe833fcf3da6a4180d181ff68d6da34f40db
Should also add coverage for `GetLevel`
```diff
diff --git a/src/txgraph.cpp b/src/txgraph.cpp
index ef7b6568107..0e7e5d22947 100644
--- a/src/txgraph.cpp
+++ b/src/txgraph.cpp
@@ -2421,6 +2421,8 @@ void TxGraphImpl::SanityCheck() const
if (cluster.GetTxCount() != 0) {
actual_clusters.insert(&cluster);
}
+
+ asse
...
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2352347809)
In "txgraph: Make level of Cluster implicit (optimization)" 22f5fe833fcf3da6a4180d181ff68d6da34f40db
Should also add coverage for `GetLevel`
```diff
diff --git a/src/txgraph.cpp b/src/txgraph.cpp
index ef7b6568107..0e7e5d22947 100644
--- a/src/txgraph.cpp
+++ b/src/txgraph.cpp
@@ -2421,6 +2421,8 @@ void TxGraphImpl::SanityCheck() const
if (cluster.GetTxCount() != 0) {
actual_clusters.insert(&cluster);
}
+
+ asse
...
💬 ismaelsadeeq commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2352500751)
In "txgraph: keep track of Cluster memory usage (preparation)" 7974ed311c1843d548d98e9eae8fed71a42a1026
Should also count the sequence number of the cluster?
```suggestion
// Memory usage of the cluster m_sequence.
+ sizeof(uint64_t);
```
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2352500751)
In "txgraph: keep track of Cluster memory usage (preparation)" 7974ed311c1843d548d98e9eae8fed71a42a1026
Should also count the sequence number of the cluster?
```suggestion
// Memory usage of the cluster m_sequence.
+ sizeof(uint64_t);
```
💬 ismaelsadeeq commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2352615744)
In "txgraph: Make level of Cluster implicit (optimization)" https://github.com/bitcoin/bitcoin/commit/22f5fe833fcf3da6a4180d181ff68d6da34f40db
Instead of using an int for level here, should we relax the `Level` `enum` from a scoped `enum` to an unscoped one to allow implicit conversion and unified input? This would also apply in cases where we want to pass a specific `enum` value, e.g. in e.g `CopyToStaging`
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2352615744)
In "txgraph: Make level of Cluster implicit (optimization)" https://github.com/bitcoin/bitcoin/commit/22f5fe833fcf3da6a4180d181ff68d6da34f40db
Instead of using an int for level here, should we relax the `Level` `enum` from a scoped `enum` to an unscoped one to allow implicit conversion and unified input? This would also apply in cases where we want to pass a specific `enum` value, e.g. in e.g `CopyToStaging`