✅ vasild closed an issue: "Use semantic analysis in lint-logs.py"
(https://github.com/bitcoin/bitcoin/issues/27825)

💬 MarcoFalke commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1682450942)
I don't think anything here is a bug fix (currently it should be a refactor). Using `fs::` is safer, as explained in the pull request description.

There shouldn't be any rush to merge this pull request before someone reviewed the code, and I am happy to rebase it every now and then (when no review happened yet) to see if there are any real (unlikely) bugs introduced.

💬 fanquake commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1682469685)
I would just like to remove the

> confusing and potentially dangerous,

code. I don't see how that is predicated on, or needs to wait for a Rust based linter. Happy to ACK the first commit.

🤔 ryanofsky requested changes to a pull request: "blockstorage: Return on fatal flush errors"
(https://github.com/bitcoin/bitcoin/pull/27866#pullrequestreview-1582770374)
Code review 7721ab9139013d70ef0f058754fabc5aaeda2246. Thanks for splitting up the commits and adding helpful log prints and comments. I like everything in this PR except the new `return false` after `FlushBlockFile()` fails in `Chainstate::FlushStateToDisk`. I think stickies-v makes the best possible case for it here: https://github.com/bitcoin/bitcoin/pull/27866#pullrequestreview-1570159243, but the test mentioned there of commenting out the block flush code entirely seems not very realistic, a
...

💬 ryanofsky commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1297313148)
re: https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1273739178

I think this `return false` should be removed and replaced by a log print and comment, at least in this PR. Maybe a followup PR could try to change behavior in a bigger way, but it's not clear here that the intended change of returning false is here actually good, and that there aren't other unintended effects.

If a `FlatFileSeq::Flush` call fails, effectively meaning that an `fflush` or `ftruncate` call failed on blo
...

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1297413491)
> > Because, most of the time, the peer manager class will call to the CConMan setter and subsequently call the getter to utilize it.
>
> I wasn't saying to make connman the container for this data, I only meant to add a setter to inform connman of the things it needs to perform its job (making connections to desirable nodes). The main container for the flags would still be peerman.

Ok. What I dislike about that approach is that we will lose a lot of flexibility.

The PeerManager should
...

💬 jonatack commented on pull request "ci: Add test-each-commit task":
(https://github.com/bitcoin/bitcoin/pull/28279#issuecomment-1682596724)
Concept ACK if our CI infra can handle it. I build and run the unit tests on each commit for more critical pulls, or pulls with commits that aren't clearly separate or that look interactive, or those with many commits, but often without also running the functional tests that take longer to run. Having the CI verify this would alert PR authors more quickly and save developer time for everyone.

💬 apoelstra commented on pull request "wallet: add `seeds` argument to `importdescriptors`":
(https://github.com/bitcoin/bitcoin/pull/27351#issuecomment-1682598823)
Rebased.

💬 jonatack commented on pull request "ci: Add test-each-commit task":
(https://github.com/bitcoin/bitcoin/pull/28279#issuecomment-1682600172)
> often without also running the functional tests that take longer to run

That said, what the build testing most often finds is a failing build before running any tests. So if infra resources are limited, just build + units might still catch most of the issues.

💬 nerd2ninja commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1682605147)
Concept Ack. To quote moonsettler from a different mempool policy discussion

https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666604137

>evil mempool is among the largest existential threats to bitcoin and we absolutely have no way to stop it from coming into existence. in fact unnecessarily restrictive mempool policy naturally incentivizes it's emergence.

>people attempting to effectively filter economically motivated, paying market for block space will most likely result in
...

💬 kevkevinpal commented on pull request "init: changing -torcontrol help to specify that a default port is used":
(https://github.com/bitcoin/bitcoin/pull/28101#issuecomment-1682610519)
rebased to [5fa09d0](https://github.com/bitcoin/bitcoin/pull/28101/commits/5fa09d039f4e920c18f089fcb1fb4df7813468fc)

🤔 jonatack reviewed a pull request: "doc: clarify cookie generation in JSON-RPC-interface.md"
(https://github.com/bitcoin/bitcoin/pull/28283#pullrequestreview-1583026183)
Concept ACK

💬 jonatack commented on pull request "doc: clarify cookie generation in JSON-RPC-interface.md":
(https://github.com/bitcoin/bitcoin/pull/28283#discussion_r1297476742)
```suggestion
- **Secure authentication:** By default, when no `-rpcpassword` configuration option is specified, Bitcoin Core generates unique
```

----

Unrelated, is this still planned?

`src/httprpc.cpp:251: LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcauth for rpcauth auth generation.\n");
`

💬 jonatack commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1297489117)
If you retouch, the transition to severity-based logging will be easier if used when adding new ones. Probably choose between error, warning, or info level here; all 3 will be logged unconditionally by default, unless the user sets a custom severity level.

```suggestion
LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "%s\n", "test"); });
LogPrintLevel(BCLog::BLOCKSTORAGE, "Failed to flush previous block file %05i (finalize=%i, finalize_undo=%i) before opening new bl
...

💬 jonatack commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1297490048)
Idem as https://github.com/bitcoin/bitcoin/pull/27866/files#r1297489117 if you retouch.

💬 sr-gi commented on pull request "net processing: avoid serving non-announced txs as a result of a MEMPOOL message":
(https://github.com/bitcoin/bitcoin/pull/27602#issuecomment-1682645449)
> Maybe the test? Though, I haven't looked at it recently.

That may make sense. I'll take a look and rebase

📝 MarcoFalke opened a pull request: "ci: Disable --coverage temporarily"
(https://github.com/bitcoin/bitcoin/pull/28285)
Looks like this causes all CI builds to be red, and doesn't work anyway, see https://github.com/bitcoin/bitcoin/issues/27593 . Temporarily disable it to allow for more time to rework it from scratch.

💬 MarcoFalke commented on pull request "init: changing -torcontrol help to specify that a default port is used":
(https://github.com/bitcoin/bitcoin/pull/28101#issuecomment-1682662462)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 jonatack commented on pull request "init: changing -torcontrol help to specify that a default port is used":
(https://github.com/bitcoin/bitcoin/pull/28101#discussion_r1297514131)
This is already included 3 lines higher.

💬 jonatack commented on pull request "init: changing -torcontrol help to specify that a default port is used":
(https://github.com/bitcoin/bitcoin/pull/28101#discussion_r1297514797)
```suggestion
constexpr int DEFAULT_TOR_CONTROL_PORT = 9051;
```