💬 maflcko commented on pull request "fuzz: Add check in `p2p_headers_presync` that chain work never exceeds minimum work":
(https://github.com/bitcoin/bitcoin/pull/30918#issuecomment-2363907360)
review ACK 284bd17309ab3b124d9dcddfec62f5506383343b

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1768765758)
I think this line got pulled out of the `else if` conditional. Won't this set the value for all unique parents and reject any transaction with two unique prevout hashes?

✅ fanquake closed a pull request: "depends: Qt 5.15.15"
(https://github.com/bitcoin/bitcoin/pull/30774)

🚀 fanquake merged a pull request: "refactor: move `SignSignature` helpers to test utils"
(https://github.com/bitcoin/bitcoin/pull/30561)

💬 l0rinc commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768725247)
I'm not sure I understand why we need this.

Why not make them all runtime instead, why are we "testing" compile time behavior here at all, aren't we already doing that in the rest of the source code?

It seems to me the whole situation would simplify a lot if we would only test runtime behavior here.

💬 l0rinc commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768784651)
I like the idea of testing the validator together with the implementation!

But I'm not in love with the current approach.

Why are we testing `PassFmt<1>("%02d");` with `foo` - I don't think it helps with understanding how formatter works.

Also `PassFmt<1>("%s")` made sense when we were only validating the number of args, but we've extended it since, I think we should extend examples with the actual parameters, e.g.
* `PassFmt("%s", "test");`
* `PassFmt("%02d", 42);`
* `PassFmt("%12$s
...

💬 tdb3 commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1768746217)
nit: If touching the file again, looks like this could be simplified.
```diff
- bool IsVersionSelected() const { return m_details_level == 3 || m_details_level >= 4; }
+ bool IsVersionSelected() const { return m_details_level >= 3; }
```

🤔 tdb3 reviewed a pull request: "netinfo: add peer services column and outbound-only peers list"
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2318488868)
Left a couple comments.
Tested that using `5` does include only outbound peers by connecting two nodes locally.

💬 tdb3 commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1768786471)
Do we expect to receive a null in `servicesnames` among other entries?
If we don't, maybe this should be something like `assert(!services[i].isNull());` so the issue can be noticed.

💬 bitcoin-tools commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2364028677)
Can it be simplified to `const std::string command{"echo err 1>&2 && false"};` or is `sh -c '...'` necessary to ensure portability?

💬 achow101 commented on pull request "cli: Improve error message on multiwallet cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2364029768)
ACK 54227e681a4efa8961f1ad05d43366d88a9b686a

💬 maflcko commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2364037617)
I do not know, but I think that `echo` and `false` would fall back to the coreutils one, if the shell built-in isn't available. The previous requirement `ls` is also from coreutils, IIUC, so it should be fine as well.

💬 Sjors commented on pull request "Stratum v2 Transport":
(https://github.com/bitcoin/bitcoin/pull/30315#issuecomment-2364044654)
Each of these CI failures seem spurious, some sort of timeout after many hours. However there's real failures in the followup PR. Going to rebase this to avoid confusion.

💬 achow101 commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1768862688)
The relative path is relative to the wallet directory, not the default wallet directory. Starting with `-walletdir=<dir>` will change the wallet directory and specifying a wallet name in this argument will be interpreted to be relative to that walletdir.

🚀 achow101 merged a pull request: "cli: Improve error message on multiwallet cli-side commands"
(https://github.com/bitcoin/bitcoin/pull/26990)

💬 fjahr commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1768873992)
done

💬 fjahr commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1768874108)
added

💬 fjahr commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1768875839)
I have amended the log message and I have added basic coverage for the balances of the wallet. That required some further changes to the test as a whole since there were no balances before due to the use of miniwallet, so I have done this in a separate commit.

💬 LarryRuane commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#issuecomment-2364098530)
@fanquake - thanks, I'll fix all these this weekend.

💬 Sjors commented on issue "Intermittent failure in p2p_1p1c_network.py", line 58, in raise_network_minfee assert_greater_than(node.getmempoolinfo()['mempoolminfee'], FEERATE_1SAT_VB) ; AssertionError: 0.00001000 <= 0.00001000":
(https://github.com/bitcoin/bitcoin/issues/30922#issuecomment-2364108949)
I saw this several times today as well.