💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1427922624)
Changed `PackageV3Checks` to properly look at ancestor and descendant sets within package, so it handles packages within testmempoolaccepts now

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1427923115)
Yes. In the new function I've added an `Assume(IsTopoSortedPackage(package))`

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1427923287)
This is gone now so marking as resolved

💬 fanquake commented on pull request "ci: Set `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK` to avoid unrelated failures":
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1857811571)
> This fix makes Homebrew skip upgrading the aws-sam-cli package that we do not use.

Ok the issue here also isn't specifically related to `aws-sam-cli`, because upgrading any Python will failure, due symlinks not being able to be written.

💬 fanquake commented on pull request "ci: Set `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK` to avoid unrelated failures":
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1857820371)
Looks like ultimately, we can't avoid the failure by removing packages we don't use.

The root cause, is that `qt@5` has a dependency on `python@3.11`, and after we install `qt@5`, brew will try and upgrade `python@3.11`, which will then fails:
```bash
==> Upgrading python@3.11
3.11.6 -> 3.11.6_1

==> Pouring python@3.11--3.11.6_1.ventura.bottle.tar.gz
Error: The `brew link` step did not complete successfully
The formula built, but is not symlinked into /usr/local
Could not symlink
...

✅ fanquake closed a pull request: "ci: uninstall aws-sam-cli"
(https://github.com/bitcoin/bitcoin/pull/29092)

💬 fanquake commented on pull request "ci: uninstall aws-sam-cli":
(https://github.com/bitcoin/bitcoin/pull/29092#issuecomment-1857821047)
Looks like this wont work, due to qt still having a dep that will fail to upgrade, see https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1857820371.

💬 Sjors commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1427941674)
If it still takes that long you probably want to pick a smaller K and/or N.

> did you mean 100-of-100? Or should I leave this out completely and just keep the 1-of-0 and 1-of-1000 tests further down that utilises the same new helper method?

The other two tests only check the failure case. A 1-of-100 would check the success path. 100-of-100 might be fine too, but maybe slower?

👍 furszy approved a pull request: "fuzz: coinselection, improve `min_viable_change`/`change_output_size`"
(https://github.com/bitcoin/bitcoin/pull/28372#pullrequestreview-1783974332)
Code ACK cd810075eddd

💬 eriknylund commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1427966127)
100-of-100 seems to run okay, less than 16 sec to complete the whole test. However, after the rebase I see the same behavior on my Mac M1 as you're seeing on your 2019 MacBook Pro. Even the 1-of-999 test doesn't pass without an RPC timeout, which it did before! So perhaps there's been some change while the PR has been open that now causes the test to perform slower? 🤔

1-of-999 gives me:
`'sendall' RPC took longer than 30.000000 seconds. Consider using larger timeout for calls that take lon
...

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1427996156)
^We figured this out offline (see passed vs accepted), thanks

💬 Sjors commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1428018720)
Interesting. Try cleaning your working directory (e.g. `git clean -dfx`) and building again. To try on an older version of master, you can you check out any old commit and then do a `git cherry-pick ...`.

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1428016068)
sure, `IsBlockRecent()` sounds good.

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1427973929)
> What is meant by "the version of the peer" here?

It is talking about the version message. `NetMsgType::VERSION`. Which informs the supported services to the other end.

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1428018613)
> Seems like `!fInitialDownload && ` can be removed? If `IsBlockCloseToTip()` is `true` then that block is 3h20min or younger. In this case `fInitialDownload` will always be `false` because it uses 24h, right?

It is a simple and quick check to not execute `IsBlockCloseToTip()` during IBD at all.

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1427974175)
sure

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1428012937)
There isn't a behavior change here. `StartExtraBlockRelayPeers()` can only be executed once, then it runs for the entire software lifetime.

I don't think the base condition is different. Both action paths depend on the proximity to the tip. The extra block-relay-only peers functionality is activated when the node is close to the tip (20 blocks away). And the "desirable service flags" is modified on the opposite direction, when the node detect it is further than 288 blocks from the tip.

I
...

💬 maflcko commented on pull request "refactor: Print verbose serialize compiler error messages":
(https://github.com/bitcoin/bitcoin/pull/29056#discussion_r1428027127)
Shouldn't `WritableStream` be applied at all implementations instead to constrain them? Otherwise, one could still call `Object{}.Serialize(WrongStream{})` in other parts of the code?

💬 maflcko commented on pull request "refactor: Print verbose serialize compiler error messages":
(https://github.com/bitcoin/bitcoin/pull/29056#discussion_r1428027208)
Thanks,

* added "s"
* Left it as `BasicByte` for now, to mirror the stdlib naming: https://en.cppreference.com/w/cpp/concepts/floating_point
* Added clang-format fields

💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1428034455)
Since we're resetting all constraints on every loop, is this not overly verbose? (+below)

<details>
<summary>git diff on 364456f659</summary>

```diff
diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
index 1400fadea0..7ddc45c4b4 100644
--- a/src/test/fuzz/tx_pool.cpp
+++ b/src/test/fuzz/tx_pool.cpp
@@ -200,17 +200,10 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
// The sum of the values of all spendable outpoints
constexpr CAmount SUPPLY_TO
...