💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1622471290)
I've rolled back this. After some of the refactoring, this looked like:

```
std::clamp(available_fds - min_required_fds, 0, nUserMaxConnections);
```

However, `available_fds` cannot be smaller than `min_required_fds`, and `nUserMaxConnections` cannot be negative. So calling `std::min` should be enough.

📝 willcl-ark opened a pull request: "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN"
(https://github.com/bitcoin/bitcoin/pull/30212)
Closes: #19363

Renaming these two errors improves clarity around the returned error when:

1. a transactions' outputs are already found in the utxo set (`ALREADY_IN_CHAIN -> ALREADY_IN_UTXO_SET`)
2. a transactions' inputs are missing from the utxo set, which could also be due to all inputs having already been spent (`MISSING_INPUTS -> INPUTS_MISSING_OR_SPENT`)

💬 willcl-ark commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#issuecomment-2142284206)
This addresses #19363 's concern:

> If a transaction is already in the block chain, and all of its outputs are spent, the BroadcastTransaction() fails to detect the correct transaction status. It returns TransactionError::MISSING_INPUTS instead of TransactionError::ALREADY_IN_CHAIN.

...by now returning `TransactionError::INPUTS_MISSING_OR_SPENT`. The [translation](https://github.com/bitcoin/bitcoin/blob/62f7f59ff495fbcbfc10c25e97bb0dc032647abf/src/util/error.cpp#L19) for this error message
...

🚀 fanquake merged a pull request: "depends: qt 5.15.14 and fix macOS build with Clang 18"
(https://github.com/bitcoin/bitcoin/pull/30198)

🚀 fanquake merged a pull request: "depends: consolidate dependency docs"
(https://github.com/bitcoin/bitcoin/pull/30204)

📝 Sjors opened a pull request: "net: have GetReceivedMessage() return bytes"
(https://github.com/bitcoin/bitcoin/pull/30213)
Fixes #30209.

Previously `GetReceivedMessage()` returned a CNetMessage. Have the caller process the bytes instead. Similarly `SetMessageToSend` needed a `CSerializedNetMsg`, give it bytes instead.

This makes the `Transport` class usable for future message types that don't inherit from CNetMessage.

TODO:
- [ ] clean up
- [ ] fix tests
- [ ] implement `SetMessageToSend`

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1622525854)
@theuni Nice idea, done.

💬 Sjors commented on issue "Make Transport independent of CNetMessage and CSerializedNetMsg":
(https://github.com/bitcoin/bitcoin/issues/30209#issuecomment-2142404400)
Easier said than done... very rough draft in #30213

💬 Sjors commented on pull request "net: have GetReceivedMessage() return bytes":
(https://github.com/bitcoin/bitcoin/pull/30213#issuecomment-2142415999)
v2 is fairly straight-forward, but v1 is not.

💬 sipa commented on pull request "net: have GetReceivedMessage() return bytes":
(https://github.com/bitcoin/bitcoin/pull/30213#discussion_r1622531541)
Please don't do this; the whole point of having separate transport classes is hiding the complexity of dealing with the difference from the rest of the network code.

💬 sipa commented on pull request "net: have GetReceivedMessage() return bytes":
(https://github.com/bitcoin/bitcoin/pull/30213#issuecomment-2142430707)
Would an alternative approach be that `V1Transport` and `V2Transport` always just treat the first 12 bytes of a message as the command type (as in on decode they'd produce a message with "length" 12+payload_len, etc). That would mean the `Transport` interface works purely on bytes, and could be usable for protocols that don't deal with things that have a command type.

Another option is having two layers: a "raw transport" interface that works on bytes, and a layer "command-based transport" in
...

📝 ryanofsky opened a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214)
This PR contains the first part of #28608, which tries to make assumeutxo code more maintainable, and improve it by not locking `cs_main` for a long time when the snapshot block is connected, and by deleting the snapshot validation chainstate when it is no longer used, instead of waiting until the next restart.

The changes in this PR are just refactoring. They make `Chainstate` objects self-contained, so for example, it is possible to determine what blocks to connect to a chainstate without q
...

💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1622550401)
Cool! Would you mind sharing the current disk usage here https://github.com/setavenger/blindbit-oracle/issues/15
it's been a while since I've done a full mainnet sync.

🤔 marcofleon reviewed a pull request: "fuzz: add more coverage for `ScriptPubKeyMan`"
(https://github.com/bitcoin/bitcoin/pull/30134#pullrequestreview-2091071703)
Tested ACK e3249f21111f1dd4beb66f10af933c34a36c30ac. I ran the updated harness for ~9 hours on an empty corpus, generated a coverage report, and checked that the new functions mentioned were hit. Coverage of `scriptpubkeyman.cpp` increased.

🤔 mzumsande reviewed a pull request: "net: log connections failures via SOCKS5 with less severity"
(https://github.com/bitcoin/bitcoin/pull/30064#pullrequestreview-2091095163)
Code Review ACK f3cfbd65f54b5b6423d786fb8850cece05af603e

💬 Sjors commented on pull request "net: have GetReceivedMessage() return bytes":
(https://github.com/bitcoin/bitcoin/pull/30213#issuecomment-2142483329)
In `Sv2Transport` (see [here](https://github.com/bitcoin/bitcoin/pull/29432/files#diff-a84ee692989f2fa3ed26266d59b1d22b608a4f2cfb0ef1faa910a5b96c8ebeef) ) the encrypted headers contains both the message length and size. In `V2Transport` we first get the size and then get the type and its content. And in `V1Transport` we first get the message type and then the size followed by the content.

At the very minimum `Transport` needs to know the message size. Currently the `V1Transport` and `V2Transp
...

💬 hebasto commented on pull request "depends: remove `FORCE_USE_SYSTEM_CLANG`":
(https://github.com/bitcoin/bitcoin/pull/30201#issuecomment-2142491843)
Concept ACK.

✅ fanquake closed a pull request: "refactor: refactored platform assignment into get_platform function"
(https://github.com/bitcoin/bitcoin/pull/29971)

💬 fanquake commented on pull request "refactor: refactored platform assignment into get_platform function":
(https://github.com/bitcoin/bitcoin/pull/29971#issuecomment-2142510334)
Thanks for the contribution, however we are going to leave this code as-is for now.

💬 fanquake commented on pull request "RFC: depends: add release type to CMake builds":
(https://github.com/bitcoin/bitcoin/pull/29962#issuecomment-2142512483)
This "seems" like the right thing to do, but I'm also not sure. Could you rebase here, to incorporate other recent CMake changes, and so it's easier to test?