📝 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`
💬 fjahr commented on pull request "rpc: add scan_utxoset option to getbalance(s) to verify wallet balance accuracy":
(https://github.com/bitcoin/bitcoin/pull/33392#issuecomment-3298984427)
It seems kind of weird to me to add this as an option to `getbalance. The problem is in the `importdescriptors` call, which is using the wrong birthdate. Have you looked into making this an option to `importdescriptors`? If the performance isn't too bad, this could even be on by default. If that is possible that would seem preferred, since you also write "However, users may not realize that their wallet balance is incomplete.", if they don't know they likely also won't call the `getbalance` with
...
(https://github.com/bitcoin/bitcoin/pull/33392#issuecomment-3298984427)
It seems kind of weird to me to add this as an option to `getbalance. The problem is in the `importdescriptors` call, which is using the wrong birthdate. Have you looked into making this an option to `importdescriptors`? If the performance isn't too bad, this could even be on by default. If that is possible that would seem preferred, since you also write "However, users may not realize that their wallet balance is incomplete.", if they don't know they likely also won't call the `getbalance` with
...
💬 glozow commented on pull request "rpc: followups for min fee changes":
(https://github.com/bitcoin/bitcoin/pull/33189#discussion_r2352734299)
Btw, I added this to the release notes draft: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/v30.0-Release-Notes-Draft#updated-rpcs
(https://github.com/bitcoin/bitcoin/pull/33189#discussion_r2352734299)
Btw, I added this to the release notes draft: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/v30.0-Release-Notes-Draft#updated-rpcs