💬 john-moffett commented on pull request "log: Check that the timestamp string is non-empty to avoid undefined behavior":
(https://github.com/bitcoin/bitcoin/pull/27317#discussion_r1146762216)
I think I've already come around to the view that it's so unlikely as to be not worth the additional LoC, so I'll switch to the simpler version.

Thanks!

💬 prusnak commented on issue "pruneblockchain should be able to increase the size of pruned blockchain":
(https://github.com/bitcoin/bitcoin/issues/24286#issuecomment-1481826196)
Ack

💬 MarcoFalke commented on issue "tidy: enable `cppcoreguidelines-pro-type-member-init`":
(https://github.com/bitcoin/bitcoin/issues/27315#issuecomment-1481831715)
This indeed looks more involved than I initially thought. Report on the cpp files only:

```diff
diff --git a/src/.clang-tidy b/src/.clang-tidy
index b2c1b49..c5379f1 100644
--- a/src/.clang-tidy
+++ b/src/.clang-tidy
@@ -1,5 +1,6 @@
Checks: '
-*,
+cppcoreguidelines-pro-type-member-init,
bugprone-argument-comment,
bugprone-use-after-move,
misc-unused-using-decls,
@@ -17,4 +18,3 @@ WarningsAsErrors: '*'
CheckOptions:
- key: performance-move-const-arg.CheckTriviallyCopyable
...

📝 brunoerg opened a pull request: "addrman, refactor: improve stochastic test in `AddSingle`"
(https://github.com/bitcoin/bitcoin/pull/27319)
In the current implementation, if `pinfo->nRefCount` is 0, we created an unnecessary variable (`nFactor`). The new approach changes it to avoid it.

💬 brunoerg commented on pull request "wallet: add config to prioritize a solution that doesn't create change in coin selection ":
(https://github.com/bitcoin/bitcoin/pull/23475#issuecomment-1481851030)
Closing for now as I haven't been working on it for a while

✅ brunoerg closed a pull request: "wallet: add config to prioritize a solution that doesn't create change in coin selection "
(https://github.com/bitcoin/bitcoin/pull/23475)

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1481881156)
Not sure why the functional tests are failing. The test failing on the Win64 job is passing on the 32-bit job and vice-versa. Both are passing locally.

💬 achow101 commented on pull request "bench: update logging benchmarks":
(https://github.com/bitcoin/bitcoin/pull/26957#issuecomment-1481883224)
ACK 8c47d599b87d6b2d43e7d37ce0aaf4f541535bb9

🚀 achow101 merged a pull request: "bench: update logging benchmarks"
(https://github.com/bitcoin/bitcoin/pull/26957)

💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1481906438)
Updated 972335762aeaab557dbb2e44b149b18005987f8b -> d00762c75a1b9ad16e0dadf25886a7baefa84a12 ([`pr/ignoredconf.6`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.6) -> [`pr/ignoredconf.7`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.7), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ignoredconf.6..pr/ignoredconf.7)) to try to fix a new win64 CI error in test_ignored_default_conf:

```
Traceback (most recent call last):
File "C:\Users\ContainerAdminis
...

💬 ryanofsky commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1481912454)
Win64 wallet_create_tx.py failure is probably fixed by #27318

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1146864188)
Thanks @vasild. It could maybe even be split further into two methods, one for the `warning` deprecations in 4 wallet RPCs, and one called by all the wallet RPCs having a `warnings` field. Will explore.

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1146874799)
s/v25/v26/

💬 theuni commented on pull request "fix: contrib: allow multi-sig binary verification":
(https://github.com/bitcoin/bitcoin/pull/23020#issuecomment-1481930724)
> https://github.com/achow101/bitcoin/tree/pr23020-direct-bins-gpg-parse is a branch which implements a subcommand for verifying binaries directly and to use `--status-file` for the machine parseable output.

Testing your branch now. Let me know if you'd like to take review elsewhere.

Sorry I don't speak much python. Some of these problems may already exist as opposed to being introduced here.

First, testing with python3.8:
`verify.py pub 23.0-x86_64`
```bash
File "/home/cory/dev/bi
...

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1146876222)
Remove

💬 ajtowns commented on pull request "p2p: remove adjusted time":
(https://github.com/bitcoin/bitcoin/pull/25908#issuecomment-1481946114)
> Rebased on master. @ajtowns putting aside the current comments, any thoughts on including changes like this in bitcoin-inquisition?

No objection to it, but not sure why it would be easier to do it that way than just merge into master?

💬 achow101 commented on pull request "fix: contrib: allow multi-sig binary verification":
(https://github.com/bitcoin/bitcoin/pull/23020#issuecomment-1481979695)
> I probably won't have time in the next few days to attend to this, so feel free to open a replacement PR with @achow101's branch if I'm holding anything up.

Unfortunately I will be away for the next two weeks soon, so I won't have any time to maintain a replacement PR either.

> First, testing with python3.8: `verify.py pub 23.0-x86_64`
>
> ```shell
> File "/home/cory/dev/bitcoin2/contrib/verifybinaries/verify.py", line 192, in parse_gpg_result
> def line_begins_with(patt: str,
...

💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1146930797)
```suggestion
// As long as target feerate is below tx6's ancestor feerate, there is no bump fee.
```

💬 theStack commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1146941331)
minor suggestion: in the unit test's `sanity_check` function, could also check that all (positive) bumpfee entries refer to one of the passed tx's:
```suggestion
// No negative bumpfees. All positive bumpfees must refer to one of the passed tx's outputs.
for (const auto& [outpoint, fee] : bumpfees) {
if (fee < 0) return false;
if (fee == 0) continue;
auto outpoint_ = outpoint; // structured bindings can't be captured in C++17, so we need to use a variable

...

💬 theStack commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1146927271)
nit: Seems like this pair of curly-braces and the extra-indent is a left-over from earlier code (probably involving a `LOCK`?) and can just be removed.