💬 AngusP commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#discussion_r1607133058)
Ah yes, I feel I should've remembered that 😆 it's usually considered correct-est to use the `%` formatting in log messages because that formatted string is only actually created if the message will be logged based on log-level, vs `f"` or `.format` or `" % ...` which are always created. Others may consider that premature optimisation if string-formatting due to logging has negligible perf impact 😉
(https://github.com/bitcoin/bitcoin/pull/30130#discussion_r1607133058)
Ah yes, I feel I should've remembered that 😆 it's usually considered correct-est to use the `%` formatting in log messages because that formatted string is only actually created if the message will be logged based on log-level, vs `f"` or `.format` or `" % ...` which are always created. Others may consider that premature optimisation if string-formatting due to logging has negligible perf impact 😉
💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1607136024)
This feels overengineered. Why can't we stick to fs::perms internally and just map the string in a function?
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1607136024)
This feels overengineered. Why can't we stick to fs::perms internally and just map the string in a function?
💬 sipa commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2121018386)
Benchmarks on my Ryzen 5950X system:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 2,373.94 | 421,240.11 | 0.1% | 1.10 | `LinearizeNoIters16TxWorstCase`
| 7,530.22 | 132,798.26 | 0.0% | 1.07 | `LinearizeNoIters32TxWorstCase`
| 16,585.34 | 60,294.20 | 0.1% | 1.10 | `LinearizeNoIters48TxWorstCase`
...
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2121018386)
Benchmarks on my Ryzen 5950X system:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 2,373.94 | 421,240.11 | 0.1% | 1.10 | `LinearizeNoIters16TxWorstCase`
| 7,530.22 | 132,798.26 | 0.0% | 1.07 | `LinearizeNoIters32TxWorstCase`
| 16,585.34 | 60,294.20 | 0.1% | 1.10 | `LinearizeNoIters48TxWorstCase`
...
💬 murchandamus commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2121032237)
Concept ACK on making the TRUC transaction vsize limit 10 kvB.
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2121032237)
Concept ACK on making the TRUC transaction vsize limit 10 kvB.
🤔 theuni reviewed a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2066845490)
Concept ACK. Nice.
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2066845490)
Concept ACK. Nice.
💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1607175446)
Any reason to not make `max_size_bytes` a `CSignatureCache` constructor param? That way `InitSignatureCache` could go away.
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1607175446)
Any reason to not make `max_size_bytes` a `CSignatureCache` constructor param? That way `InitSignatureCache` could go away.
🤔 theuni reviewed a pull request: "build: Remove `--enable-threadlocal`"
(https://github.com/bitcoin/bitcoin/pull/30137#pullrequestreview-2066875638)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30137#pullrequestreview-2066875638)
Concept ACK
🤔 mzumsande reviewed a pull request: "validation: sync chainstate to disk after syncing to tip"
(https://github.com/bitcoin/bitcoin/pull/15218#pullrequestreview-2066847859)
Concept ACK
While this one-time sync after IBD should help in some situations, I'm not sure that it completely resolves https://github.com/bitcoin/bitcoin/issues/11600 (I encountered this PR while looking into possible improvements to `ReplayBlocks()`)
After all, there are several other situations in which a crash / unclean shutdown could lead to extensive replays (e.g. during IBD) that this PR doesn't address.
(https://github.com/bitcoin/bitcoin/pull/15218#pullrequestreview-2066847859)
Concept ACK
While this one-time sync after IBD should help in some situations, I'm not sure that it completely resolves https://github.com/bitcoin/bitcoin/issues/11600 (I encountered this PR while looking into possible improvements to `ReplayBlocks()`)
After all, there are several other situations in which a crash / unclean shutdown could lead to extensive replays (e.g. during IBD) that this PR doesn't address.
💬 mzumsande commented on pull request "validation: sync chainstate to disk after syncing to tip":
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1607166628)
Should the original scheduling be conditional on `IsInitialBlockDownload() == true`? Seems like the goal of this PR is to trigger flush if we transition out of IBD, but not so much after restarting an almost or completely synced node. Currently, it will sync 1 minute after restart (with the second call to `SyncCoinsTipAfterChainSync`, because for the first one after 30s `last_chain_height` is still -1).
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1607166628)
Should the original scheduling be conditional on `IsInitialBlockDownload() == true`? Seems like the goal of this PR is to trigger flush if we transition out of IBD, but not so much after restarting an almost or completely synced node. Currently, it will sync 1 minute after restart (with the second call to `SyncCoinsTipAfterChainSync`, because for the first one after 30s `last_chain_height` is still -1).
🤔 murchandamus reviewed a pull request: "policy: restrict all TRUC (v3) transactions to 10KvB"
(https://github.com/bitcoin/bitcoin/pull/29873#pullrequestreview-2066839394)
ACK 0583aeef4f2b09f35547f95f1fe21dc14738b65d
Nit in PR title and second commit message:
Uppercase _K_ kilo refers to 1024¹ per the JEDEC standard, the symbol of the metric prefix _kilo_ for 1000¹ is lowercase _k_ (presumably because the symbol _K_ was already used for Kelvin in the SI system). Since you are limiting TRUC transactions to 10,000 vB, I think you mean "kvB" instead of "KvB".
(https://github.com/bitcoin/bitcoin/pull/29873#pullrequestreview-2066839394)
ACK 0583aeef4f2b09f35547f95f1fe21dc14738b65d
Nit in PR title and second commit message:
Uppercase _K_ kilo refers to 1024¹ per the JEDEC standard, the symbol of the metric prefix _kilo_ for 1000¹ is lowercase _k_ (presumably because the symbol _K_ was already used for Kelvin in the SI system). Since you are limiting TRUC transactions to 10,000 vB, I think you mean "kvB" instead of "KvB".
💬 murchandamus commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1607161860)
Typo:
```suggestion
// descendant limit descendants. The transaction also cannot be v3,
// as its topology restrictions do not allow a second child.
```
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1607161860)
Typo:
```suggestion
// descendant limit descendants. The transaction also cannot be v3,
// as its topology restrictions do not allow a second child.
```
💬 murchandamus commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1607176526)
<s>I don’t understand the test here. My understanding was that TRUC transactions would be generally limited to 10 kvB, and TRUC transactions with a parent would be limited to 1 kvB. It seems to me that they should together be allowed to weigh up to 11 kvB, so it is no clear to me why this comment reads "but parent's descendant limit is exceeded".</s>
Perhaps clarify here in the comment that there is a custom descendant limit in play:
```suggestion
# Parent and child are within v3 li
...
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1607176526)
<s>I don’t understand the test here. My understanding was that TRUC transactions would be generally limited to 10 kvB, and TRUC transactions with a parent would be limited to 1 kvB. It seems to me that they should together be allowed to weigh up to 11 kvB, so it is no clear to me why this comment reads "but parent's descendant limit is exceeded".</s>
Perhaps clarify here in the comment that there is a custom descendant limit in play:
```suggestion
# Parent and child are within v3 li
...
💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2121088705)
Am I right that after this change (or alternatives which let BatchWrite just iterate dirty/fresh entries), it would be possible to (perhaps dramatically) reduce the periodic flush time, so that after IBD, we basically always have a mostly non-dirty cache?
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2121088705)
Am I right that after this change (or alternatives which let BatchWrite just iterate dirty/fresh entries), it would be possible to (perhaps dramatically) reduce the periodic flush time, so that after IBD, we basically always have a mostly non-dirty cache?
🤔 furszy reviewed a pull request: "indexes: Don't wipe indexes again when continuing a prior reindex"
(https://github.com/bitcoin/bitcoin/pull/30132#pullrequestreview-2067063831)
Looking good in a first glance. It would be nice to add some coverage for it. Maybe assert that certain logs are not present during init? Like the "Wiping LevelDB in <index_path>" one.
(https://github.com/bitcoin/bitcoin/pull/30132#pullrequestreview-2067063831)
Looking good in a first glance. It would be nice to add some coverage for it. Maybe assert that certain logs are not present during init? Like the "Wiping LevelDB in <index_path>" one.
💬 hebasto commented on pull request "build: Remove `--enable-threadlocal`":
(https://github.com/bitcoin/bitcoin/pull/30137#issuecomment-2121304986)
> Concept ACK.
I forgot to mention that it would be great to ditch it before CMake :)
(https://github.com/bitcoin/bitcoin/pull/30137#issuecomment-2121304986)
> Concept ACK.
I forgot to mention that it would be great to ditch it before CMake :)
👍 cbergqvist approved a pull request: "Make it possible to disable Tor binds and abort startup on bind failure"
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2066219143)
ACK d0c8109dd0f2c8ca3ee742f0a0f11911553e0a62
Was worried that the UI might fail to show the MessageBoxes about bind errors in `CConnman::Bind()` now that `CConnman::InitBinds()` fails on bind errors instead of continuing. But MessageBoxes are modal due to `MSG_ERROR = (ICON_ERROR | BTN_OK | MODAL)` and I manually verified it works fine in *bitcoin-qt* by attempting to bind to a port occupied by another process on my machine.
Ran functional `--extended` tests with one single *expected* test
...
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2066219143)
ACK d0c8109dd0f2c8ca3ee742f0a0f11911553e0a62
Was worried that the UI might fail to show the MessageBoxes about bind errors in `CConnman::Bind()` now that `CConnman::InitBinds()` fails on bind errors instead of continuing. But MessageBoxes are modal due to `MSG_ERROR = (ICON_ERROR | BTN_OK | MODAL)` and I manually verified it works fine in *bitcoin-qt* by attempting to bind to a port occupied by another process on my machine.
Ran functional `--extended` tests with one single *expected* test
...
💬 cbergqvist commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1607299718)
The `self.expected` variable name really confused me since it includes command line inputs used to *steer* the test, but it was named prior to this PR. Maybe improve the situation with some slightly clearer names in the loop?
```python
for i, (args, expected_services) in enumerate(self.expected):
self.log.info(f"Checking listening ports of node {i} with {args}")
```
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1607299718)
The `self.expected` variable name really confused me since it includes command line inputs used to *steer* the test, but it was named prior to this PR. Maybe improve the situation with some slightly clearer names in the loop?
```python
for i, (args, expected_services) in enumerate(self.expected):
self.log.info(f"Checking listening ports of node {i} with {args}")
```
💬 cbergqvist commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1606777724)
nit: Introducing style mismatch on adjacent lines - mixing single and double quotes in the two `addr_to_hex()` calls.
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1606777724)
nit: Introducing style mismatch on adjacent lines - mixing single and double quotes in the two `addr_to_hex()` calls.
💬 cbergqvist commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1607158855)
nit: Using `''.format()` instead of `f''` as recommended in https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines
> Use f'{x}' for string formatting in preference to '{}'.format(x) or '%s' % x.
Prior entries use the recommended style apart from `"`-quotes.
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1607158855)
nit: Using `''.format()` instead of `f''` as recommended in https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines
> Use f'{x}' for string formatting in preference to '{}'.format(x) or '%s' % x.
Prior entries use the recommended style apart from `"`-quotes.
💬 hebasto commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2121349899)
This branch has been rebased on top of the https://github.com/hebasto/bitcoin/pull/202.
I've noticed two issues so far:
1. `Cannot open mapping file '/ci_container_base/ci/scratch/build-/contrib/devtools/iwyu/bitcoin.core.imp': No such file or directory.`
2. Missed disabling `EXPORT_COMPILE_COMMANDS` property for targets in the `crc32c` subtree.
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2121349899)
This branch has been rebased on top of the https://github.com/hebasto/bitcoin/pull/202.
I've noticed two issues so far:
1. `Cannot open mapping file '/ci_container_base/ci/scratch/build-/contrib/devtools/iwyu/bitcoin.core.imp': No such file or directory.`
2. Missed disabling `EXPORT_COMPILE_COMMANDS` property for targets in the `crc32c` subtree.