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

💬 instagibbs commented on pull request "tests: Don't depend on value of DEFAULT_PERMIT_BAREMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/29088#issuecomment-1858033912)
crACK https://github.com/bitcoin/bitcoin/pull/29088/commits/7b45744df33c6a4759eae1a3984f389cbac837c2

(someone should probably switch it default false and check)

💬 ajtowns commented on pull request "refactor: Print verbose serialize compiler error messages":
(https://github.com/bitcoin/bitcoin/pull/29056#discussion_r1428102823)
Yeah, that's probably reasonable

📝 ajtowns opened a pull request: "NOMERGE UFG Default permitbaremultisig=0"
(https://github.com/bitcoin/bitcoin/pull/29093)
Cherry-picks patches from #28217 on top of #29088 so that CI can verify 29088 does what it says on the box

💬 maflcko commented on pull request "tests: Don't depend on value of DEFAULT_PERMIT_BAREMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/29088#issuecomment-1858091384)
> (someone should probably switch it default false and check)

done with

```dif
diff --git a/src/policy/policy.h b/src/policy/policy.h
index 6a7980c312..42359f12a5 100644
--- a/src/policy/policy.h
+++ b/src/policy/policy.h
@@ -36,7 +36,7 @@ static constexpr unsigned int DEFAULT_INCREMENTAL_RELAY_FEE{1000};
/** Default for -bytespersigop */
static constexpr unsigned int DEFAULT_BYTES_PER_SIGOP{20};
/** Default for -permitbaremultisig */
-static constexpr bool DEFAULT_PERMIT_BAREM
...

📝 maflcko opened a pull request: " ci: Better tidy errors "
(https://github.com/bitcoin/bitcoin/pull/29094)

💬 ajtowns commented on issue "Discussion: Upgrading to C++20":
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1858134567)
> > Having `std::span<>` with the size as part of the type rather than a runtime thing could be nice for the various crypto things where we currently do things like `assert(key.size() == KEYLEN)`.
>
> I guess this requires a helper method to safely convert to a fixed-size span, internally preferring a compile-time check, or falling back to an assert, if it isn't available?

It doesn't /require/ it, but it'd probably be sensible so we always get an assertion failure rather than corrupted mem
...