💬 maflcko commented on pull request "doc: Add `nproc` support for Mac through `coreutils`":
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2363354174)
No objection to adding this, but just installing it and then leaving the docs in this file as `# Use "-j N" here for N parallel jobs.` doesn't seem too useful for most users, assuming that devs already have it installed anyway (or an alternative).
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2363354174)
No objection to adding this, but just installing it and then leaving the docs in this file as `# Use "-j N" here for N parallel jobs.` doesn't seem too useful for most users, assuming that devs already have it installed anyway (or an alternative).
💬 fanquake commented on pull request "doc: Add `nproc` support for Mac through `coreutils`":
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2363362007)
I think the only place this might make sense would be in the developer documentation. I don't think we should be adding additional things as base dependencies for os specific instructions, just so someone can call a utility. It's also less relevant, given devs may already be using generators that are handling this by default, i.e Ninja.
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2363362007)
I think the only place this might make sense would be in the developer documentation. I don't think we should be adding additional things as base dependencies for os specific instructions, just so someone can call a utility. It's also less relevant, given devs may already be using generators that are handling this by default, i.e Ninja.
💬 fanquake commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768352268)
2 lines above, this already says `Assuming the build directory is named build,`
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768352268)
2 lines above, this already says `Assuming the build directory is named build,`
💬 fanquake commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768350617)
Same here. I don't think we need to duplicate `(assuming build is your build directory)` 10 lines apart. Especially when all the build commands here used `build` as the build dir.
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768350617)
Same here. I don't think we need to duplicate `(assuming build is your build directory)` 10 lines apart. Especially when all the build commands here used `build` as the build dir.
💬 fanquake commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768350553)
You're adding `(assuming build is your build directory)` here, but 3 lines up it already says `Assuming the build directory is named build,`.
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768350553)
You're adding `(assuming build is your build directory)` here, but 3 lines up it already says `Assuming the build directory is named build,`.
💬 fanquake commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768351361)
You can't modify the subtree here.
(https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1768351361)
You can't modify the subtree here.
💬 l0rinc commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2363371383)
Unsurprisingly (should we document this?), setting `-prune` makes the `-dbcache` value less important:
<details>
<summary>benchmark</summary>
```bash
// 6d7f24595b08b8d1eba53e648533bcf87c30b48f
hyperfine \
--runs 2 \
--export-json /mnt/ibd_dbcache_prune.json \
--parameter-list DBCACHE 10240,16384,20480,30720,40960 \
--prepare 'rm -rf /mnt/BitcoinData/*' \
'./build/src/bitcoind -datadir=/mnt/BitcoinData -printtoconsole=0 -stopatheight=860000 -prune=550 -dbcache={DBCACHE}'
```
</
...
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2363371383)
Unsurprisingly (should we document this?), setting `-prune` makes the `-dbcache` value less important:
<details>
<summary>benchmark</summary>
```bash
// 6d7f24595b08b8d1eba53e648533bcf87c30b48f
hyperfine \
--runs 2 \
--export-json /mnt/ibd_dbcache_prune.json \
--parameter-list DBCACHE 10240,16384,20480,30720,40960 \
--prepare 'rm -rf /mnt/BitcoinData/*' \
'./build/src/bitcoind -datadir=/mnt/BitcoinData -printtoconsole=0 -stopatheight=860000 -prune=550 -dbcache={DBCACHE}'
```
</
...
💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768357254)
Done in 2955b1a1f3eec934960e880963a09b359d828721.
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768357254)
Done in 2955b1a1f3eec934960e880963a09b359d828721.
💬 l0rinc commented on pull request "doc: Add `nproc` support for Mac through `coreutils`":
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2363381454)
Thanks for the comments, would it make sense to add this to the mac developer notes instead, or should we remove the `-j N` docs, since people are migrating to Ninja anyway?
This stupid `nproc` problem comes up often for macs, was hoping we can finally get rid of it...
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2363381454)
Thanks for the comments, would it make sense to add this to the mac developer notes instead, or should we remove the `-j N` docs, since people are migrating to Ninja anyway?
This stupid `nproc` problem comes up often for macs, was hoping we can finally get rid of it...
📝 fanquake unlocked a pull request: "refactor, kernel: Remove gArgs accesses from dbwrapper and txdb"
(https://github.com/bitcoin/bitcoin/pull/25862)
Code in the libbitcoin_kernel library should not be calling `ArgsManager` methods or trying to read options from the command line. Instead it should just get options values from simple structs and function arguments that are passed in externally. This PR removes `gArgs` accesses from `dbwrapper` and `txdb` modules by defining appropriate options structs, and is a followup to PR's #25290 #25487 #25527 which remove other `ArgsManager` calls from kernel modules.
This PR does not change behavior
...
(https://github.com/bitcoin/bitcoin/pull/25862)
Code in the libbitcoin_kernel library should not be calling `ArgsManager` methods or trying to read options from the command line. Instead it should just get options values from simple structs and function arguments that are passed in externally. This PR removes `gArgs` accesses from `dbwrapper` and `txdb` modules by defining appropriate options structs, and is a followup to PR's #25290 #25487 #25527 which remove other `ArgsManager` calls from kernel modules.
This PR does not change behavior
...
💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768368784)
Yes, exactly. Depending on how https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767054547 turns out, this test-only pull will have to be adjusted.
There are already 4 different follow-ups, all of which explicitly or implicitly conflict with each other.
I don't really want to open a 5th one to remove the linter first.
The more pull requests exist that explicitly or implicitly conflict with each other, the more review will be wasted, because if one of them is merged, all others
...
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768368784)
Yes, exactly. Depending on how https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767054547 turns out, this test-only pull will have to be adjusted.
There are already 4 different follow-ups, all of which explicitly or implicitly conflict with each other.
I don't really want to open a 5th one to remove the linter first.
The more pull requests exist that explicitly or implicitly conflict with each other, the more review will be wasted, because if one of them is merged, all others
...
💬 maflcko commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2363412596)
Looks like this is still a problem. The macOS 14 task was able to finish in less than 30 minutes, even without a ccache at all.
Now it is stuck again for more than 90 minutes: https://github.com/bitcoin/bitcoin/actions/runs/10956545011/job/30422825062?pr=30935#step:7:3249
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2363412596)
Looks like this is still a problem. The macOS 14 task was able to finish in less than 30 minutes, even without a ccache at all.
Now it is stuck again for more than 90 minutes: https://github.com/bitcoin/bitcoin/actions/runs/10956545011/job/30422825062?pr=30935#step:7:3249
🤔 stickies-v reviewed a pull request: "log: Enforce trailing newline at compile time"
(https://github.com/bitcoin/bitcoin/pull/30929#pullrequestreview-2317918165)
Concept ACK on these checks being done at compile-time instead of through clang-tidy/linters/..., and the significant resulting code clean-up. Added verbosity can be hidden, e.g. by the alias suggested as a nit.
(https://github.com/bitcoin/bitcoin/pull/30929#pullrequestreview-2317918165)
Concept ACK on these checks being done at compile-time instead of through clang-tidy/linters/..., and the significant resulting code clean-up. Added verbosity can be hidden, e.g. by the alias suggested as a nit.
💬 stickies-v commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1768397353)
nit: might be nice introducing a `LogFmt` alias here?
<details>
<summary>git diff on fa7190db83</summary>
```diff
diff --git a/src/logging.h b/src/logging.h
index e0bd79f793..ed224565e6 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -239,7 +239,10 @@ static inline bool LogAcceptCategory(BCLog::LogFlags category, BCLog::Level leve
bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str);
template <typename... Args>
-inline void LogPrintFormatInternal(std::string_vie
...
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1768397353)
nit: might be nice introducing a `LogFmt` alias here?
<details>
<summary>git diff on fa7190db83</summary>
```diff
diff --git a/src/logging.h b/src/logging.h
index e0bd79f793..ed224565e6 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -239,7 +239,10 @@ static inline bool LogAcceptCategory(BCLog::LogFlags category, BCLog::Level leve
bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str);
template <typename... Args>
-inline void LogPrintFormatInternal(std::string_vie
...
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2363454663)
Fixed ci
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2363454663)
Fixed ci
📝 theStack opened a pull request: "scripted-diff: drop config/ subdir for bitcoin-config.h"
(https://github.com/bitcoin/bitcoin/pull/30937)
This PR is a follow-up to #30856, as suggested in comment https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2356804690. With the scripted diff, review should be fairly trivial, but it could still be seen as controversial due to the large number of files (78 in total) being touched.
(https://github.com/bitcoin/bitcoin/pull/30937)
This PR is a follow-up to #30856, as suggested in comment https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2356804690. With the scripted diff, review should be fairly trivial, but it could still be seen as controversial due to the large number of files (78 in total) being touched.
💬 ismaelsadeeq commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1768427965)
nit
```suggestion
self.log.info("Test loading backup on a pruned node when the backup was created close to the prune height of the restoring node")
```
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1768427965)
nit
```suggestion
self.log.info("Test loading backup on a pruned node when the backup was created close to the prune height of the restoring node")
```
💬 ismaelsadeeq commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1768424398)
nit:
Assert that the chain height is 214, because the test assume so?
```suggestion
# Ensure the chain tip is at height 214, because this test assume it is.
assert_equal(node.getchaintips()[0]["height"], 214)
# We need a few more blocks so we can actually get above an realistic
```
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1768424398)
nit:
Assert that the chain height is 214, because the test assume so?
```suggestion
# Ensure the chain tip is at height 214, because this test assume it is.
assert_equal(node.getchaintips()[0]["height"], 214)
# We need a few more blocks so we can actually get above an realistic
```
🤔 ismaelsadeeq reviewed a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678#pullrequestreview-2318004044)
code review ACK 38648f4940eb13ebc858081c63e587156428107a
nice adding 38648f4940eb13ebc858081c63e587156428107a 👍🏾
(https://github.com/bitcoin/bitcoin/pull/30678#pullrequestreview-2318004044)
code review ACK 38648f4940eb13ebc858081c63e587156428107a
nice adding 38648f4940eb13ebc858081c63e587156428107a 👍🏾
💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768447740)
While posting this PR I was aware of your `tuple_cat` suggestion. I am not against it on principle but TBH I haven't had a clear reason to struggle through that corner of C++ and don't see your suggestion compelling enough to work through it. Your suggestion in that comment is close to compiling but doesn't handle sending in one arg less for the failure cases. A motivation behind this PR is to prove parity in a clear way, and I'm worried it might obfuscate it. Happy to learn more from a diff.
...
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768447740)
While posting this PR I was aware of your `tuple_cat` suggestion. I am not against it on principle but TBH I haven't had a clear reason to struggle through that corner of C++ and don't see your suggestion compelling enough to work through it. Your suggestion in that comment is close to compiling but doesn't handle sending in one arg less for the failure cases. A motivation behind this PR is to prove parity in a clear way, and I'm worried it might obfuscate it. Happy to learn more from a diff.
...