✅ MarcoFalke closed an issue: "lint: MYPY_CACHE_DIR is unused"
(https://github.com/bitcoin/bitcoin/issues/28183)

💬 MarcoFalke commented on issue "test: RPC coverage check doesn't work?":
(https://github.com/bitcoin/bitcoin/issues/27593#issuecomment-1682392919)
Looks like there are also other bugs with this: https://cirrus-ci.com/task/6126543575973888?logs=ci#L5061

💬 fanquake commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#issuecomment-1682397002)
@sipa when you get a chance to respond to review comments here, we can decide to either merge as-is, and address in a followup, or touch up here.

💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1682399338)
> > I think you asking these questions inappropriately gives readers the impression
> > that there actually are non-trivial examples of real businesses who are relying
> > on unconfirmed transactions. The fact is in this entire thread no-one has been
> > able to provide clear, convincing, evidence that this is true.
>
> I think you have two layers of reasoning: a) browsing the matrix of alternatives solutions for unconfirmed transactions and b) the economics traffic of the use-cases which
...

💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#issuecomment-1682414932)
I think they should either be fixed up here or not at all. I don't think it makes sense to create a follow-up to a follow-up.

💬 sipa commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#issuecomment-1682416176)
I will address them.

💬 fanquake commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1682417462)
I don't think we are going to get the Rust linter over the line here. Obviously the filesystem commit is correct, and should be merged. Do you want to split that out, and continue the lint discussion, or drop the linter from this PR for now?

💬 MarcoFalke commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1682432081)
> Do you want to split that out, and continue the lint discussion

Seems better to keep the discussion in one place for now.

💬 fanquake commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1682437724)
> Seems better to keep the discussion in one place for now.

Ok, but the discussion of introducing Rust into the codebase (in whatever form) is clearly contentious, and shouldn't be a blocker for merging obvious bug fixes.

💬 Sjors commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1297349342)
I think `!node.m_sock` can only happen when when we've disconnected in another thread, using `CloseSocketDisconnect()` (and during tests). That's also what the documentation of `m_sock_mutex` says. cc @vasild

👍 Sjors approved a pull request: "net: transport abstraction"
(https://github.com/bitcoin/bitcoin/pull/28165#pullrequestreview-1582634664)
ACK d9a91ba9606f828ef3846d2877ec56313653a109

💬 Sjors commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1297229209)
58cb50cdc60df5d7e42466213f213cf7ab54db45: IIUC in v1 `&& pnode->vSendMsg.empty()` is always `true` when `GetBytesToSend()` just told us there's nothing to send? But in v2 we might be waiting for a handshake.

💬 vasild commented on issue "Use semantic analysis in lint-logs.py":
(https://github.com/bitcoin/bitcoin/issues/27825#issuecomment-1682443788)
Fixed by https://github.com/bitcoin/bitcoin/pull/26296, thanks!

✅ 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.