💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#issuecomment-2388482275)
Are you still working on this?

I am asking now, because there shouldn't be any conflicts after rebase, I think.

💬 l0rinc commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2388503588)
I did a few benchmarks on HDD and SSD separately (no raspberry pi yet, but I understood @davidgumberg did some of those and saw a significant speedup), to see the effect of the different values on IBD.

I have tried different values via https://github.com/bitcoin/bitcoin/pull/30059 (rebased), namely DBFILESIZE 1,__2__,4,8,16,32,64,128,256,512 (current value is 2) with default `dbcache`, until 600k blocks using real nodes (which introduces some randomness, but the repeated runs should still ind
...

🤔 l0rinc reviewed a pull request: "Add option dbfilesize to control LevelDB target ("max") file size"
(https://github.com/bitcoin/bitcoin/pull/30059#pullrequestreview-2342721830)
I've rebased this and ran a few benchmarks with different values.
Will do a few more measurements, but so far I'm rather leaning on finding a better value than the default instead of making this confuggurable.
See: https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2388503588

💬 l0rinc commented on pull request "Add option dbfilesize to control LevelDB target ("max") file size":
(https://github.com/bitcoin/bitcoin/pull/30059#discussion_r1784374410)
setting the option here after we've partially constructed it seems inconsequential, could we add it as a parameter instead?
```diff
diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp
--- a/src/dbwrapper.cpp (revision 5f69dc11cb53c1794e9e71e6f398b95c1037cdc2)
+++ b/src/dbwrapper.cpp (date 1727870700414)
@@ -134,9 +134,10 @@
options->max_open_files, default_open_files);
}

-static leveldb::Options GetOptions(size_t nCacheSize)
+static leveldb::Options GetOptions(size_t nC
...

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2388526922)
Building 291ec5e2a876622a051baf6e33a4d4d592e6568e (on arm64):
```bash
make -C depends HOST=x86_64-apple-darwin qt
Building native_qt...
[13/312] Building CXX object qtbase/src/tools/macdeployqt/macdeployqt/CMakeFiles/macdeployqt.dir/__/shared/shared.cpp.o
FAILED: qtbase/src/tools/macdeployqt/macdeployqt/CMakeFiles/macdeployqt.dir/__/shared/shared.cpp.o
/Library/Developer/CommandLineTools/usr/bin/clang++ -isysroot/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk -DGL_SILENCE_DEPRECATION
...

💬 dergoegge commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2388558223)
Concept ACK

This option seems useful for users that want to:

* prevent passive spying on their connections
* increase the cost of tampering with the data exchanged on their connections
* increase the cost of detecting that they run a bitcoin node

> Currently 59.71% of network supports v2

Have you checked how diverse these ~60% of v2 supporting nodes are? An extreme example: if they are all on running on AWS then I don't think we should add the option at this time.

💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2388568119)
@hebasto now I can't build QT with depends anymore (291ec5e2a876622a051baf6e33a4d4d592e6568e): https://gist.github.com/Sjors/e42242b27b71c98ee10d60d58a20872a

(since the last time I tested I uninstalled Xcode as part of debugging #30978)

💬 josibake commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2388619208)
Echoing others, I like option 2 (side-binaries). A `bitcoin <cmd>` wrapper on top of multiple binaries also seems quite nice. While this might cause some confusion for end users, I think this will be far less confusing than having a side release (and much less work for maintainers and guix builders).

Regarding option 3, one of the value propositions of multiprocess in my mind is being able to run a binary for a specific purpose, with only the code needed for that purpose in the binary, e.g.,
...

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2388665904)
reACK ce205abe10ebccfdbea60adf4c568a8ba61390c3

thanks for picking up on some of those old nits

📝 maflcko opened a pull request: "test: Treat exclude list warning as failure in CI"
(https://github.com/bitcoin/bitcoin/pull/31018)
An outdated exclude list or otherwise an error in the exclude list handling is usually a bug.

So make it fatal in the CI, instead of silently ignoring it.

Fixes https://github.com/bitcoin/bitcoin/pull/30872/files#r1757015334

Can be tested with something like (with and without `--ci`):

```
./bld-cmake/test/functional/test_runner.py wallet_disable -x wallet_disablee

💬 maflcko commented on pull request "test: fix exclude parsing for functional runner":
(https://github.com/bitcoin/bitcoin/pull/30872#discussion_r1784546679)
Fixed in https://github.com/bitcoin/bitcoin/pull/31018

💬 josibake commented on issue "rfc: DATUM mining interface requirements":
(https://github.com/bitcoin/bitcoin/issues/31002#issuecomment-2388674235)
I think this highlights a clear advantage of having a more "generic" mining interface exposed over an IPC interface: different mining protocols can use the same interface from Bitcoin Core, without needing protocol specific[^1] changes for each protocol to be implemented in Bitcoin Core.

As you mentioned, our current interface is designed with SV2 in mind, but given this is all relatively new code, it would be great to hear from the folks building DATUM whether or not this interface works out
...

💬 jonatack commented on pull request "doc: update IBD requirements in doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/30992#issuecomment-2388680568)
> The commit message was created automatically when I clicked the "Commit Suggestion" button on your previous feedback. Fixing it now.

@Mackain The commit message wasn't updated yet, could you update it and re-push the same commit (thanks!)

💬 maflcko commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2388688159)
Please report the CI failure. Otherwise, it will be harder to track and fix it. This is explained in the log:

```
test 2024-10-02T00:55:45.897000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
test 2024-10-02T00:55:45.897000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues

🤔 hodlinator reviewed a pull request: "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error"
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2342962102)
stickies-v:
> All format strings are known at compile-time, so any formatting errors are programming errors. Handling that with `util::Result` would be both a cumbersome interface and bad practice imo.

This is a good argument except we do have some strings only known at runtime, notably translations, which probably don't get full testing. Maybe there's something that automatically compares format strings for equivalence across translations though.

maflcko:
> I agree that `util::Result` d
...

💬 hodlinator commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1784535464)
Changing tinyformat to support `string_view` is definitely out of scope for this PR.

I would at least want to make it clearer that format strings are separate from other parameters, like this or with other words to that effect:
```suggestion
- *Rationale*: Tinyformat handles string parameters. Format strings
on the other hand should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
```
Even though I decided
...

💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#issuecomment-2388722857)
@maflcko
> Are you still working on this?

I'm waiting on #30928 before un-drafting this.

💬 maflcko commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2388728933)
Concept ACK

💬 l0rinc commented on pull request "cmake: Avoid hardcoding Qt's major version in Find module":
(https://github.com/bitcoin/bitcoin/pull/31010#discussion_r1784608370)
It seems to me we're still hardcoding the major version - can't we change this during migration instead?

💬 maflcko commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2388768319)
> This is a good argument except we do have some strings only known at runtime, notably translations, which probably don't get full testing. Maybe there's something that automatically compares format strings for equivalence across translations though.

If translations are malformed to induce a tinyformat error where one wouldn't be present in the non-translated string, I don't think the solution would to use `Result`. This seems no different from an otherwise fully tinyformat-valid string that
...