💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2248546746)
Benchmark results on my ryzen 7995wx:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,474.01 | 678,422.17 | 0.3% | 0.01 | `LinearizeNoIters16TxWorstCaseAnc`
| 1,473.48 | 678,667.50 | 0.2% | 0.01 | `LinearizeNoIters16TxWorstCaseLIMO`
| 4,763.02 | 209,951.04 | 0.1% | 0.01 | `LinearizeNoIters32TxWo
...
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2248546746)
Benchmark results on my ryzen 7995wx:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,474.01 | 678,422.17 | 0.3% | 0.01 | `LinearizeNoIters16TxWorstCaseAnc`
| 1,473.48 | 678,667.50 | 0.2% | 0.01 | `LinearizeNoIters16TxWorstCaseLIMO`
| 4,763.02 | 209,951.04 | 0.1% | 0.01 | `LinearizeNoIters32TxWo
...
💬 ryanofsky commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2248589227)
re: https://github.com/bitcoin/bitcoin/pull/30509#issue-2425377062
> The default socket path is currently `<datadir>/sockets/bitcoin-node.sock`, but this can be customized.
I changed the default socket path to `<datadir>/node.sock` after running into some "exceeded maximum socket path length" errors. The maximum unix socket path length is only 107 characters, so I thought a shorter suffix might avoid problems on systems with long data directory or home directory paths.
Another possible
...
(https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2248589227)
re: https://github.com/bitcoin/bitcoin/pull/30509#issue-2425377062
> The default socket path is currently `<datadir>/sockets/bitcoin-node.sock`, but this can be customized.
I changed the default socket path to `<datadir>/node.sock` after running into some "exceeded maximum socket path length" errors. The maximum unix socket path length is only 107 characters, so I thought a shorter suffix might avoid problems on systems with long data directory or home directory paths.
Another possible
...
👍 willcl-ark approved a pull request: "Fee Estimation: change `estimatesmartfee` default mode to `economical`"
(https://github.com/bitcoin/bitcoin/pull/30275#pullrequestreview-2197582258)
ACK 25bf86a225b0df3f48ade1016b47f5ee1636b988
Looks good to me with those nits fixed 👍🏼
(https://github.com/bitcoin/bitcoin/pull/30275#pullrequestreview-2197582258)
ACK 25bf86a225b0df3f48ade1016b47f5ee1636b988
Looks good to me with those nits fixed 👍🏼
💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1690313716)
Attempted cleaning that up, but didn't uncover anything useful to the end users, unlike #30444, so probably not worth following up on:
```diff
diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
index 89c03c1647..dab8b81128 100644
--- a/src/bitcoin-tx.cpp
+++ b/src/bitcoin-tx.cpp
@@ -274,8 +274,8 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu
// extract and validate vout
const std::string& strVout = vStrInputParts[1];
- int64_t vou
...
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1690313716)
Attempted cleaning that up, but didn't uncover anything useful to the end users, unlike #30444, so probably not worth following up on:
```diff
diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
index 89c03c1647..dab8b81128 100644
--- a/src/bitcoin-tx.cpp
+++ b/src/bitcoin-tx.cpp
@@ -274,8 +274,8 @@ static void MutateTxAddInput(CMutableTransaction& tx, const std::string& strInpu
// extract and validate vout
const std::string& strVout = vStrInputParts[1];
- int64_t vou
...
💬 mzumsande commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1690338607)
nit: After this, the log
`Committed X changed transaction outputs (out of Y) to coin database...`
has become less interesting because we only iterate over dirty coins, so `X=Y` always.
I might be more helpful to report the initial size of `m_map` of the cursor (before entries are deleted from it) as the total.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1690338607)
nit: After this, the log
`Committed X changed transaction outputs (out of Y) to coin database...`
has become less interesting because we only iterate over dirty coins, so `X=Y` always.
I might be more helpful to report the initial size of `m_map` of the cursor (before entries are deleted from it) as the total.
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1690361434)
There are other places that could be cleaned up, such as obviated dirty checks like [here](https://github.com/bitcoin/bitcoin/pull/28280/files#diff-cafbe1353eff6084b73fd3b6c3dee603e0827348fdd2fe12dfad1e01003a84edR118) and [here](https://github.com/bitcoin/bitcoin/pull/28280/files#diff-f0ed73d62dae6ca28ebd3045e5fc0d5d02eaaacadb4c2a292985a3fbd7e1c77cR195), but this should probably done after a https://github.com/bitcoin/bitcoin/pull/18746 revival so there's no confusion.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1690361434)
There are other places that could be cleaned up, such as obviated dirty checks like [here](https://github.com/bitcoin/bitcoin/pull/28280/files#diff-cafbe1353eff6084b73fd3b6c3dee603e0827348fdd2fe12dfad1e01003a84edR118) and [here](https://github.com/bitcoin/bitcoin/pull/28280/files#diff-f0ed73d62dae6ca28ebd3045e5fc0d5d02eaaacadb4c2a292985a3fbd7e1c77cR195), but this should probably done after a https://github.com/bitcoin/bitcoin/pull/18746 revival so there's no confusion.
💬 mzumsande commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1690365906)
yes, no need to do that here. I just mentioned the logging here because I was interested in the ratios to assess the efficiency of this PR, and had to write my own log.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1690365906)
yes, no need to do that here. I just mentioned the logging here because I was interested in the ratios to assess the efficiency of this PR, and had to write my own log.
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1690367507)
Also, that line predates #17487, back when all entries were dirty anyways.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1690367507)
Also, that line predates #17487, back when all entries were dirty anyways.
👍 TheCharlatan approved a pull request: "guix: GCC 12 consolidation"
(https://github.com/bitcoin/bitcoin/pull/30511#pullrequestreview-2197680394)
ACK d1592d2eee1913e734a4f92907e796eb3350c64a
Guix build (aarch64):
```
4fba0aba8be59fd967b099e19faf6ba6770a83de1e7bee745c16f91fa3eb92fe guix-build-d1592d2eee19/output/aarch64-linux-gnu/SHA256SUMS.part
37e93c9235db88f58e5a45e8a54e1d57602f9107cd23676c91252509b2fe3bb5 guix-build-d1592d2eee19/output/aarch64-linux-gnu/bitcoin-d1592d2eee19-aarch64-linux-gnu-debug.tar.gz
184fb61be0843b069c4c0d92119b9e29e895f11ef8687b220ab3898999c06afa guix-build-d1592d2eee19/output/aarch64-linux-gnu/bitcoin-d
...
(https://github.com/bitcoin/bitcoin/pull/30511#pullrequestreview-2197680394)
ACK d1592d2eee1913e734a4f92907e796eb3350c64a
Guix build (aarch64):
```
4fba0aba8be59fd967b099e19faf6ba6770a83de1e7bee745c16f91fa3eb92fe guix-build-d1592d2eee19/output/aarch64-linux-gnu/SHA256SUMS.part
37e93c9235db88f58e5a45e8a54e1d57602f9107cd23676c91252509b2fe3bb5 guix-build-d1592d2eee19/output/aarch64-linux-gnu/bitcoin-d1592d2eee19-aarch64-linux-gnu-debug.tar.gz
184fb61be0843b069c4c0d92119b9e29e895f11ef8687b220ab3898999c06afa guix-build-d1592d2eee19/output/aarch64-linux-gnu/bitcoin-d
...
💬 TheCharlatan commented on pull request "guix: GCC 12 consolidation":
(https://github.com/bitcoin/bitcoin/pull/30511#discussion_r1690370855)
Hope this gets merged soon so we can drop this again.
(https://github.com/bitcoin/bitcoin/pull/30511#discussion_r1690370855)
Hope this gets merged soon so we can drop this again.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690385057)
Done.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690385057)
Done.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690385537)
Added the mermaid code as comments to the benchmark.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690385537)
Added the mermaid code as comments to the benchmark.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690385632)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690385632)
Fixed.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690385744)
Done.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690385744)
Done.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690386555)
I've added comments explaining this, and also moved the "test correspondence between Cluster and DepGraph" to a new `VerifyDepGraphFromCluster` function that's invoked in both the unit and fuzz tests.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690386555)
I've added comments explaining this, and also moved the "test correspondence between Cluster and DepGraph" to a new `VerifyDepGraphFromCluster` function that's invoked in both the unit and fuzz tests.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690386667)
Done.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690386667)
Done.
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2248844497)
added fuzz and unit tests which cover more than the functional tests can alone
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2248844497)
added fuzz and unit tests which cover more than the functional tests can alone
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690386739)
Done.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1690386739)
Done.
💬 instagibbs commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#issuecomment-2248854763)
reACK https://github.com/bitcoin/bitcoin/pull/30275/commits/25bf86a225b0df3f48ade1016b47f5ee1636b988
tests make me feel much better about this thanks
(https://github.com/bitcoin/bitcoin/pull/30275#issuecomment-2248854763)
reACK https://github.com/bitcoin/bitcoin/pull/30275/commits/25bf86a225b0df3f48ade1016b47f5ee1636b988
tests make me feel much better about this thanks
💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2248939831)
Was able to simplify this by dropping CustomBuildMessage/CustomReadMessage functions and tests. It now requires the latest version of the libmultiprocess library though.
Rebased 79cfc0d198faecec598f923e6bf83ccf8dc09bbb -> 6cccb436557c58e0ccd21ffe0eaf31058f5cb799 ([`pr/mine-types.2`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.2) -> [`pr/mine-types.3`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-ty
...
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2248939831)
Was able to simplify this by dropping CustomBuildMessage/CustomReadMessage functions and tests. It now requires the latest version of the libmultiprocess library though.
Rebased 79cfc0d198faecec598f923e6bf83ccf8dc09bbb -> 6cccb436557c58e0ccd21ffe0eaf31058f5cb799 ([`pr/mine-types.2`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.2) -> [`pr/mine-types.3`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-ty
...
💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2248990387)
Rebased fba04d7756c77ed9371c5dc90d3bebc9aa767f4b -> ec5f45b9186184cac9c64ed08b8b8cc596e301a3 ([`pr/mine.7`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.7) -> [`pr/mine.8`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.7-rebase..pr/mine.8)), moving most code that was here into base PRs #30509 and #30510. I also followed up various review suggestions above like fixing the top-level make target and dropping unneeded
...
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2248990387)
Rebased fba04d7756c77ed9371c5dc90d3bebc9aa767f4b -> ec5f45b9186184cac9c64ed08b8b8cc596e301a3 ([`pr/mine.7`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.7) -> [`pr/mine.8`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.7-rebase..pr/mine.8)), moving most code that was here into base PRs #30509 and #30510. I also followed up various review suggestions above like fixing the top-level make target and dropping unneeded
...