🤔 ismaelsadeeq reviewed a pull request: "Cluster size 2 package rbf"
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-2014725944)
Continued Reviewing.

- [x] 826ec4c47c0d18a2d9b437b33ea4ceba67e870c1
- [x] 033736bcd9fc16e244e52e72fe7c7ff030690ece

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1574739231)
This also implies that [single conflict package RBF carvout](https://github.com/bitcoin/bitcoin/pull/23711#discussion_r765766768) is not supported, is this worth mentioning?

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1574741503)
```suggestion
# Ensure an individual transaction with single direct conflict can RBF the chain which used our carve-out rule

```

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1574766371)
In `packages.md` there is note stating "Replace By Fee is currently disabled for packages." This is no longer the case, some variants of package can now be RBF'd I think we should indicate that we now support cluster size 2 package replacement into node's mempool stating the new acceptance rules.

💬 maflcko commented on pull request "test: Fix intermittent timeout in p2p_tx_download.py":
(https://github.com/bitcoin/bitcoin/pull/29933#issuecomment-2069448527)
To get the full error log on Cirrus CI, you'll have to use the "Open Full Logs" button to see the full log. Usually the output is:

... (dots from the test runner)
traceback (from the failed test)
Full combined logs (from the failed test)
Full test summary (from the test runner)

💬 sipa commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#issuecomment-2069471388)
Rebased after the merge of #29879.

💬 jrakibi commented on pull request "test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply":
(https://github.com/bitcoin/bitcoin/pull/29617#discussion_r1574776450)
Done 👍

💬 glozow commented on pull request "test: Fix intermittent timeout in p2p_tx_download.py":
(https://github.com/bitcoin/bitcoin/pull/29933#issuecomment-2069499720)
I can see the `self.shutdown()` traceback in this one: https://api.cirrus-ci.com/v1/task/6740372997537792/logs/ci.log

💬 jrakibi commented on pull request "test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply":
(https://github.com/bitcoin/bitcoin/pull/29617#issuecomment-2069508021)
@fjahr I've updated the code based on your feedback. In the commit description, you can find the steps followed to arrive at the content values, along with a tool to decode a UTXO snapshot

✅ maflcko closed an issue: "Error unlocking wallet: some keys decrypt but not all. your wallet file may be corrupt. "
(https://github.com/bitcoin/bitcoin/issues/29789)

💬 instagibbs commented on pull request "policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only)":
(https://github.com/bitcoin/bitcoin/pull/29843#issuecomment-2069528455)
> Seems like this goes beyond the expected behaviour. Maybe make it =2 at least?

Since these are test-network-only changes, are we required to preserve current behavior?

💬 theuni commented on pull request "[WIP] libevent @ master + use CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2069538101)
I believe (@fanquake can confirm) that the idea was to test the full merge, and if we liked the CMake build parts we'd patch them in and drop the rest.

💬 Sjors commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-2069559533)
My remark is orthogonal to #29918. macOS < 13 doesn't have clang 15. They also don't have clang 14, which was missed in #29208.

There's need to make a separate pull request for a single sentence:

At the bottom of step 3:

```md
On macOS < 13 you additionally need to install a more recent version of clang:

brew install clang
```

💬 fjahr commented on pull request "test: Fix multiprocess CI timeout in p2p_tx_download":
(https://github.com/bitcoin/bitcoin/pull/29926#discussion_r1574814671)
Had another failure, so I will add the +300 back and see if that is a permanent fix.

💬 maflcko commented on pull request "test: Fix intermittent timeout in p2p_tx_download.py":
(https://github.com/bitcoin/bitcoin/pull/29933#issuecomment-2069568540)
> I can see the `self.shutdown()` traceback in this one: https://api.cirrus-ci.com/v1/task/6740372997537792/logs/ci.log

Thanks, in this case it seems the issue is transactions relayed to the extraneous peers. I've edited the pull request description.

To see what message is the cause for the block, you can inspect the full combined log. In this example:


```
...
[0;36m test 2024-04-19T18:08:46.732000Z TestFramework.p2p (DEBUG): Received message from 127.0.0.1:17361: msg_tx(tx=CTrans
...

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-2069591081)
> There's need to make a separate pull request for a single sentence:

I don't think this is enough. Is it not required to specify the CC and CXX to `./configure`? Something like `CC=$(brew --prefix llvm)/bin/clang CXX=$(brew --prefix llvm)/bin/clang++`? Or does brew overwrite the clang system compiler?

Another reason to fix this in a separate pull is to have the fix in before this pull is merged (which at this point is unclear when it can happen).

💬 maflcko commented on pull request "test: Fix multiprocess CI timeout in p2p_tx_download":
(https://github.com/bitcoin/bitcoin/pull/29926#issuecomment-2069610577)
> Example failure: https://cirrus-ci.com/task/5622109341220864

If I download https://api.cirrus-ci.com/v1/task/5622109341220864/logs/ci.log, I get

```
2024-04-21T00:10:09.801000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
wait_until_helper_internal(lambda: not self.network_event_loop.is_running(), timeout=timeout)
'''
[node 1] Cleaning up leftover process
[node 0] Cleaning up leftover process


[1mstderr:
[0mTraceback (most recent call last):
Fi
...

📝 Sjors opened a pull request: "doc: add LLVM instruction for macOS < 13"
(https://github.com/bitcoin/bitcoin/pull/29934)
#29208 bumped clang to 14, which users of old macOS versions need to install manually. This PR adds instructions.

Xcode 14.3.1 ships clang 14.0.3:
https://en.wikipedia.org/wiki/Xcode#Xcode_11.0_-_14.x_(since_SwiftUI_framework)

The system requirements for that is macOS Ventura 13.0 or later: https://developer.apple.com/documentation/xcode-release-notes/xcode-14_3_1-release-notes#

Homebrew itself officially supports macOS 12 or later, but _may_ still work on macOS 11: https://docs.brew.s
...

💬 instagibbs commented on pull request "test: Fix intermittent timeout in p2p_tx_download.py":
(https://github.com/bitcoin/bitcoin/pull/29933#discussion_r1574874712)
Took me way too long to realize it's the `fill_mempool` function causing the sync timeout specifically. Can a note be left here to say why we're not making additional inbounds?

💬 Sjors commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#issuecomment-2069707168)
Done in #29934.

Correction to version support above: clang 15.0 is shipped with Xcode 15, not 14. But its system requirements are fairly similar, macOS 13.5 or better: https://developer.apple.com/documentation/xcode-release-notes/xcode-15-release-notes

I don't have a macOS 11 or 12 machine to actually test if it still works. Given that homebrew dropped explicit support for macOS 11 I don't think we need to worry about that version, but it would be nice if someone can test if they can still
...