💬 fjahr commented on pull request "index: Force database compaction in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33306#issuecomment-3254606614)
Good question and I am definitely not a LevelDB expert either. I think that at the size and usage pattern of the coinstatsindex we are not really facing any significant problems on a modern computer and system since the size of the data we are storing is very small relatively speaking for what LevelDB is capable of handling. But I think the impact is probably also non-zero and might increase over time as the blockchain grows. Very high-level rationale: there must be a reason LevelDB does these c
...
(https://github.com/bitcoin/bitcoin/pull/33306#issuecomment-3254606614)
Good question and I am definitely not a LevelDB expert either. I think that at the size and usage pattern of the coinstatsindex we are not really facing any significant problems on a modern computer and system since the size of the data we are storing is very small relatively speaking for what LevelDB is capable of handling. But I think the impact is probably also non-zero and might increase over time as the blockchain grows. Very high-level rationale: there must be a reason LevelDB does these c
...
💬 fjahr commented on pull request "index: Force database compaction in coinstatsindex":
(https://github.com/bitcoin/bitcoin/pull/33306#issuecomment-3254623438)
FWIW, from the patterns I have observed so far it seems likely to me that the problem is only occurring during sync and afterwards the data of new blocks are actually compacted as it should, just those 55kb files are left over from the sync phase. So I am thinking about changing the approach to only compact once when the index is being set to `m_synced = true`, though this would make it a more complex change and I need to do some more testing if my anecdotal observation is correct.
(https://github.com/bitcoin/bitcoin/pull/33306#issuecomment-3254623438)
FWIW, from the patterns I have observed so far it seems likely to me that the problem is only occurring during sync and afterwards the data of new blocks are actually compacted as it should, just those 55kb files are left over from the sync phase. So I am thinking about changing the approach to only compact once when the index is being set to `m_synced = true`, though this would make it a more complex change and I need to do some more testing if my anecdotal observation is correct.
💬 maflcko commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2322828365)
thx, all done
(https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2322828365)
thx, all done
🤔 janb84 reviewed a pull request: "ci: Checkout latest merged pulls"
(https://github.com/bitcoin/bitcoin/pull/33303#pullrequestreview-3186383766)
ACK fab00ebbc74fe55c41b2d531a964cdb083b02ab0
changes sinds last ACK:
- changes related to use YAML KEYS / templating.
Kinda in doubt about the use of YAML_KEY_01, in code it makes it clear that it is a YAML key but in the action execution it is visible as a `env` variable :
<details>
``` YAML
• Run actions/checkout@v5 with:
ref: refs/pull/33303/merge repository: bitcoin/bitcoin token: ***
ssh-strict: true ssh-user: git
persist-credentials: true clean: true
sparse-checkout-cone
...
(https://github.com/bitcoin/bitcoin/pull/33303#pullrequestreview-3186383766)
ACK fab00ebbc74fe55c41b2d531a964cdb083b02ab0
changes sinds last ACK:
- changes related to use YAML KEYS / templating.
Kinda in doubt about the use of YAML_KEY_01, in code it makes it clear that it is a YAML key but in the action execution it is visible as a `env` variable :
<details>
``` YAML
• Run actions/checkout@v5 with:
ref: refs/pull/33303/merge repository: bitcoin/bitcoin token: ***
ssh-strict: true ssh-user: git
persist-credentials: true clean: true
sparse-checkout-cone
...
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#issuecomment-3254821483)
I had missed a few sources of memory usage in the numbers posted in the PR description before. I've redone the derivation, and explained it in more detail. Thanks @glozow for trying to make sense of my incorrect numbers.
(https://github.com/bitcoin/bitcoin/pull/33157#issuecomment-3254821483)
I had missed a few sources of memory usage in the numbers posted in the PR description before. I've redone the derivation, and explained it in more detail. Thanks @glozow for trying to make sense of my incorrect numbers.
👍 hebasto approved a pull request: "ci: Checkout latest merged pulls"
(https://github.com/bitcoin/bitcoin/pull/33303#pullrequestreview-3186457692)
ACK fab00ebbc74fe55c41b2d531a964cdb083b02ab0.
This time, it took GitHub ~20min to update the reference.
(https://github.com/bitcoin/bitcoin/pull/33303#pullrequestreview-3186457692)
ACK fab00ebbc74fe55c41b2d531a964cdb083b02ab0.
This time, it took GitHub ~20min to update the reference.
💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3254910524)
@ryanofsky
You may be interested in these changes.
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3254910524)
@ryanofsky
You may be interested in these changes.
💬 mzumsande commented on issue "natpmp: quiet down unconditional logging":
(https://github.com/bitcoin/bitcoin/issues/33301#issuecomment-3254930519)
> How about a exponential backoff, similar to how it's done in torcontrol.cpp?
That was just a random association, I don't know enough about PCP specifics to judge if it makes much sense.
Other questions would be:
- Does this situation deserve an unconditional warning at all instead of just logging to `net` - how actionable is the warning? (it seems to occur up for multiple people)
- Is it worth trying every 5 minutes forever instead of giving up after a number of tries? Maybe just log uncondi
...
(https://github.com/bitcoin/bitcoin/issues/33301#issuecomment-3254930519)
> How about a exponential backoff, similar to how it's done in torcontrol.cpp?
That was just a random association, I don't know enough about PCP specifics to judge if it makes much sense.
Other questions would be:
- Does this situation deserve an unconditional warning at all instead of just logging to `net` - how actionable is the warning? (it seems to occur up for multiple people)
- Is it worth trying every 5 minutes forever instead of giving up after a number of tries? Maybe just log uncondi
...
💬 fanquake commented on issue "natpmp: quiet down unconditional logging":
(https://github.com/bitcoin/bitcoin/issues/33301#issuecomment-3254938999)
cc @willcl-ark
(https://github.com/bitcoin/bitcoin/issues/33301#issuecomment-3254938999)
cc @willcl-ark
🤔 maflcko reviewed a pull request: "index: Fix coinstats overflow"
(https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-3186533494)
> Hm, not clear to me why clang-tidy CI complains here about lines (or even files) I haven't touched in this PR. But that's what the last fixup commit I just pushed is for.
I haven't checked this, but the new struct is no longer trivially copyable, so it may have effects outside of the lines you have touched.
<details><summary>a diff</summary>
we should probably make it trivially copyable, but this seems unrelated.
```diff
diff --git a/src/arith_uint256.h b/src/arith_uint256.h
inde
...
(https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-3186533494)
> Hm, not clear to me why clang-tidy CI complains here about lines (or even files) I haven't touched in this PR. But that's what the last fixup commit I just pushed is for.
I haven't checked this, but the new struct is no longer trivially copyable, so it may have effects outside of the lines you have touched.
<details><summary>a diff</summary>
we should probably make it trivially copyable, but this seems unrelated.
```diff
diff --git a/src/arith_uint256.h b/src/arith_uint256.h
inde
...
💬 maflcko commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2323036958)
this looks like a false positive in clang-tidy and this looks like the wrong fix. `utxo_stats` is mutable, and taking a const reference does not make it immutable.
(In languages with a borrow checker, this wouldn't even compile, see https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b8c78ddbe26b30473498b64a47ba2a6b)
The correct fix is to disable the clang-tidy rule in this line, or globally.
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2323036958)
this looks like a false positive in clang-tidy and this looks like the wrong fix. `utxo_stats` is mutable, and taking a const reference does not make it immutable.
(In languages with a borrow checker, this wouldn't even compile, see https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b8c78ddbe26b30473498b64a47ba2a6b)
The correct fix is to disable the clang-tidy rule in this line, or globally.
📝 glozow opened a pull request: "doc: archive v29.1 release notes"
(https://github.com/bitcoin/bitcoin/pull/33309)
(https://github.com/bitcoin/bitcoin/pull/33309)
💬 maflcko commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3254968070)
> Kinda in doubt about the use of YAML_KEY_01, in code it makes it clear that it is a YAML key but in the action execution it is visible as a `env` variable :
I know, but I don't know any alternative. See also the GitHub schema validator on a previous run: https://github.com/bitcoin/bitcoin/actions/runs/17471191651/workflow:
```
.github/workflows/ci.yml
Invalid workflow file
(Line: 21, Col: 1): Unexpected value 'checkout_ref_tmpl'
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3254968070)
> Kinda in doubt about the use of YAML_KEY_01, in code it makes it clear that it is a YAML key but in the action execution it is visible as a `env` variable :
I know, but I don't know any alternative. See also the GitHub schema validator on a previous run: https://github.com/bitcoin/bitcoin/actions/runs/17471191651/workflow:
```
.github/workflows/ci.yml
Invalid workflow file
(Line: 21, Col: 1): Unexpected value 'checkout_ref_tmpl'
💬 maflcko commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2320936283)
nit: pip is now unused and can be removed
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2320936283)
nit: pip is now unused and can be removed
💬 maflcko commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323069799)
I don't think this is `-b`. You'll have to use `--revision` for raw commit ids? See https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---revisionrev
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2323069799)
I don't think this is `-b`. You'll have to use `--revision` for raw commit ids? See https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---revisionrev
💬 darosior commented on issue "natpmp: quiet down unconditional logging":
(https://github.com/bitcoin/bitcoin/issues/33301#issuecomment-3254998385)
cc @laanwj
(https://github.com/bitcoin/bitcoin/issues/33301#issuecomment-3254998385)
cc @laanwj
💬 hebasto commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3254999297)
@jesterhodl
> > checkpoints
>
> Don't see it yet, I suppose it needs a sync? <img alt="image" width="587" height="187" src="https://private-user-images.githubusercontent.com/103882255/485108528-9efcb5a1-8b8c-4544-ab53-af574f2784c7.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NTcwMDk3NDksIm5iZiI6MTc1NzAwOTQ0OSwicGF0aCI6Ii8xMDM4ODIyNTUvNDg1MTA4NTI4LTllZmNiNWExLThiOGMtNDU0NC1hYjUzLWFmNTc0ZjI3O
...
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3254999297)
@jesterhodl
> > checkpoints
>
> Don't see it yet, I suppose it needs a sync? <img alt="image" width="587" height="187" src="https://private-user-images.githubusercontent.com/103882255/485108528-9efcb5a1-8b8c-4544-ab53-af574f2784c7.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NTcwMDk3NDksIm5iZiI6MTc1NzAwOTQ0OSwicGF0aCI6Ii8xMDM4ODIyNTUvNDg1MTA4NTI4LTllZmNiNWExLThiOGMtNDU0NC1hYjUzLWFmNTc0ZjI3O
...
🤔 hebasto reviewed a pull request: "depends: strip when installing qt binaries"
(https://github.com/bitcoin/bitcoin/pull/33304#pullrequestreview-3186600277)
My Guix build:
```
x86_64
c805e4ee4a80b3b03919d280a32f272426811470646aca1e3da0a60255347738 guix-build-c9d5f211c119/output/aarch64-linux-gnu/SHA256SUMS.part
58f02ff3f18cd39e9379d0896680c9f7b6a3544a02362bb98570b95131f9d8cf guix-build-c9d5f211c119/output/aarch64-linux-gnu/bitcoin-c9d5f211c119-aarch64-linux-gnu-debug.tar.gz
5432eaaacb722bb998c2fe76e56e62ef728f60bc7fb27d50dd9ca08dc4648c09 guix-build-c9d5f211c119/output/aarch64-linux-gnu/bitcoin-c9d5f211c119-aarch64-linux-gnu.tar.gz
997f4c68a
...
(https://github.com/bitcoin/bitcoin/pull/33304#pullrequestreview-3186600277)
My Guix build:
```
x86_64
c805e4ee4a80b3b03919d280a32f272426811470646aca1e3da0a60255347738 guix-build-c9d5f211c119/output/aarch64-linux-gnu/SHA256SUMS.part
58f02ff3f18cd39e9379d0896680c9f7b6a3544a02362bb98570b95131f9d8cf guix-build-c9d5f211c119/output/aarch64-linux-gnu/bitcoin-c9d5f211c119-aarch64-linux-gnu-debug.tar.gz
5432eaaacb722bb998c2fe76e56e62ef728f60bc7fb27d50dd9ca08dc4648c09 guix-build-c9d5f211c119/output/aarch64-linux-gnu/bitcoin-c9d5f211c119-aarch64-linux-gnu.tar.gz
997f4c68a
...
💬 janb84 commented on pull request "ci: Checkout latest merged pulls":
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3255010434)
> > Kinda in doubt about the use of YAML_KEY_01, in code it makes it clear that it is a YAML key but in the action execution it is visible as a `env` variable :
>
> I know, but I don't know any alternative. See also the GitHub schema validator on a previous run: https://github.com/bitcoin/bitcoin/actions/runs/17471191651/workflow:
>
> ```
> .github/workflows/ci.yml
>
> Invalid workflow file
>
> (Line: 21, Col: 1): Unexpected value 'checkout_ref_tmpl'
> ```
Isn't this because of
...
(https://github.com/bitcoin/bitcoin/pull/33303#issuecomment-3255010434)
> > Kinda in doubt about the use of YAML_KEY_01, in code it makes it clear that it is a YAML key but in the action execution it is visible as a `env` variable :
>
> I know, but I don't know any alternative. See also the GitHub schema validator on a previous run: https://github.com/bitcoin/bitcoin/actions/runs/17471191651/workflow:
>
> ```
> .github/workflows/ci.yml
>
> Invalid workflow file
>
> (Line: 21, Col: 1): Unexpected value 'checkout_ref_tmpl'
> ```
Isn't this because of
...
👍 hebasto approved a pull request: "depends: strip when installing qt binaries"
(https://github.com/bitcoin/bitcoin/pull/33304#pullrequestreview-3186608053)
ACK c9d5f211c119268d776af282dfd1e8b7590aaf56.
(https://github.com/bitcoin/bitcoin/pull/33304#pullrequestreview-3186608053)
ACK c9d5f211c119268d776af282dfd1e8b7590aaf56.