💬 sipa commented on issue "Proposal for a new mempool design":
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1551974824)
> My understanding was that doing an optimal sort for a given cluster, requires it to have a few dozen transactions.

It's the opposite really. A cluster with 1 transaction is always optimally ordered (because only one order exists). The bigger a cluster (in number of transactions) is, the harder it is to find the optimal ordering. Up to 15-20 transactions we may be able to find it usually an exponential-time algorithm; above that we need to either just use the ancestor-feerate based order,
...

💬 sdaftuar commented on issue "Proposal for a new mempool design":
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1551974872)
@t-bast Thanks for your comments!

> The commitment transaction may be as large as MAX_STANDARD_TX_WEIGHT, so we want the cluster size to allow that (but it's obvious that the limit needs to be higher than that anyway).

FYI -- I was thinking that the easiest approach here would be if we can cap cluster vbytes to the ancestor/descendant size limits of 101kvB, or just above the max standard transaction weight. This would have the benefit of not making block construction any less efficient (d
...

💬 Sjors commented on issue "Use muhash for assumeUTXO snapshot ":
(https://github.com/bitcoin/bitcoin/issues/27669#issuecomment-1551982585)
> However, a rolling UTXO set hash is incompatible with assumeutxo commitment schemes that involve chunking snapshots (discussed below) and so the resulting assumeutxo value might have to be a tuple consisting of `(rolling_set_hash, split_snapshot_chunks_merkle_root)`.

This is what I was getting at above. You just need a second hash that _is_ order dependent, which could be the sha256 hash of the `.dat` file, a torrent hash or the `split_snapshot_chunks_merkle_root`. I don't think that needs
...

💬 pinheadmz commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1552010405)
@stickies-v @mzumsande thanks for youre review and feedback, I refactored the PR so we evict a random peer when forced if all other peers are protected. It is MUCH simpler ;-)

🤔 stickies-v reviewed a pull request: "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full"
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1431107177)
Light approach ACK c826187b5070ce89edcde0536183714a1c2e3207

I think this random selection approach suggested by mzumsande is a more elegant approach. "Light" in approach ACK because I need to build more confidence that this doesn't introduce new attack vectors.

💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1197024384)
This block can be skipped if `!force`

```suggestion
if (vEvictionCandidates.size() > 0 && force) {
```

💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1196774113)
More than before this PR (thanks mzumsande for correcting my misunderstanding on IRC), the order in which these calls to erase elements are executed matters, as it directly affects which node is unprotected with `force==true`. Perhaps it's worth add a line of documentation to mention that the most important criteria need to be protected first?

💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1197030985)
Since `force_evict` is already an `optional` defaulting to `nullopt`, this can be simplified in conjunction with my other suggestion.
```suggestion
if (vEvictionCandidates.empty()) return force_evict;
```

💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1197025555)
nit:
```suggestion
size_t randpos{FastRandomContext().randrange(vEvictionCandidates.size())};
```

💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1197031821)
Documenting the `force` parameter (or linking to `AttemptToEvictConnection`) would be helpful I think.

💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1197037590)
Imo, "find" makes it sound like this has no side effects, whereas I think this actually does disconnect the node.

💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1197041677)
suggested rephrasing (although I'm not sure about the "NoBan" qualifier, maybe we have a better established term?)
```suggestion
* @param[in] force If all connections are otherwise protected, still evict a random inbound NoBan node if it exists
```

💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1196896354)
Oh, good point @LarryRuane. I didn't realize `NodeId` was just a typedef for `int64_t`. I think that entirely resolves it indeed.

💬 stickies-v commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1196853062)
To add: I think this can be reached when e.g. all of `vEvictionCandidates` gets protected by the `ProtectNoBanConnections(vEvictionCandidates);` call?

💬 mzumsande commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1197060760)
Maybe just copy over the doc from net.cpp? That's basically what I did in a WIP [branch](https://github.com/mzumsande/bitcoin/commit/1aaa795e3f43caf8aa06331e61b14b777a04a6d9).

💬 theuni commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1552154137)
Converting to draft while I investigate the clang issue.

📝 theuni converted_to_draft a pull request: "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM"
(https://github.com/bitcoin/bitcoin/pull/27676)
This (I believe) resolves the last of the blockers for [switching us away from cctools and instead using out-of-the-box llvm and lld](https://github.com/bitcoin/bitcoin/pull/21778) for building Darwin binaries.

For now, we continue building with a pre-packaged llvm and cctools, but after this PR the clang+lld combo should just work for anyone trying it. Additionally after this PR, the new runtime `fixup_chains` behavior will be in-use, as ld64 uses it as well.

The commits may seem unrelate
...

💬 achow101 commented on issue "test: use-of-uninitialized-value in sqlite3Strlen30":
(https://github.com/bitcoin/bitcoin/issues/27222#issuecomment-1552238004)
This looks to be an issue with either sqlite or clang, I don't think there's anything that we can do to resolve these other than suppressing them.

MSan is tripping on sqlite calling `strlen` on a string that it copied into a temp buffer. That temp buffer was malloc'd by sqlite (without clearing out the data, so it has uninitialized memory), and then our string copied into it, maybe modified (this particular function normalizes a path), a null terminator added, and the result strlen'd. That la
...

🤔 theStack reviewed a pull request: "assumeutxo (2)"
(https://github.com/bitcoin/bitcoin/pull/27596#pullrequestreview-1431794725)
Did the following more or less random test scenario (build at commit b8fd765a06998a79ef6efef5355c956a4679df6f) over the last days:
- start up `bitcoind -prune=800 -blockfilterindex=1` (-dbcache intentionally left at default)
- load snapshot from height 788000 via `bitcoind loadtxoutset /path/to/utxo-788000.dat` (thanks for providing the file!)
- wait until snapshot chainstate reaches network tip
- shut down the node and wait 1-2 days (with the idea to again provoke both chainstates being in
...

💬 theStack commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1197159021)
Right now if a non-existing file path is passed, a rather cryptic error message appears:
```
$ ./src/bitcoin-cli loadtxoutset jslkdf
error code: -1
error message:
CAutoFile::operator>>: file handle is nullptr: unspecified iostream_category error
```

Should ensure that `afile` is not null and throw an explicit error if it is (like `dumptxoutset` already does), e.g.:
```suggestion
CAutoFile afile{file, SER_DISK, CLIENT_VERSION};
if (afile.IsNull()) {
throw JSONRPCErro
...