🤔 jonatack reviewed a pull request: "test: check backup from `migratewallet` can be successfully restored"
(https://github.com/bitcoin/bitcoin/pull/28257#pullrequestreview-1581364835)
Post-merge ACK.

For the getwalletinfo['balance'] check, RPC getbalances['mine']['trusted'] should work as well.

💬 jonatack commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296393528)
Thank you, I recalled there was something like that. Will do.

🤔 mzumsande reviewed a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1581277414)
Concept ACK
This seems to be similar to the suggestion by gmaxwell in https://github.com/bitcoin/bitcoin/pull/10387#issuecomment-343357330
It also is a good thing to have `protocol.h` not depend on dynamic `net_processing` state.

💬 mzumsande commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1296335745)
The comment ("shortcut for...") was helpful for me, any reason to drop it?

💬 mzumsande commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1296375222)
`m_initial_sync_finished` is set using AdjustedTime (see `CanDirectFetch()`) while it's unset using local time. In spite of plans to get rid of AdjustedTime, I think it would be better to use it here as well, otherwise there could be constant switching back and forth in some situations where the two times differ.

Also, why introduce a new magic number of 290, can't we just use `NODE_NETWORK_LIMITED_MIN_BLOCKS` (maybe + 2, if there is a reason for it)?

💬 mzumsande commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1296355296)
Why stop doing extra block-relay peers in this situation? These aren't attempted during original IBD because we are expected to be behind the tip. However, in this super-stale situation which should really not happen normally, it seems that they could only help?

💬 jonatack commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296405960)
I didn't change the level from the current `LogPrintf` since 236618061a445d2cb11e722cfac5fdae5be26abb in 2017. Since this shouldn't normally occur, unconditionally logging seems helpful.

💬 russeree commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1681249694)
Another vector that could be used to circumvent restrictions on p2ms or even p2pk relaying would be to use p2wsh. Using a p2wsh tx outpoint would allow users to embed up to 256 bits (_32 bytes_) of arbitrary data per. These outpoints would also be unprunable.

The usable data storage capabilities of p2wsh are equal to p2pk with 32 bytes per outpoint. This would increase the number of required outpoints by 3x compared to p2ms.

The existence of p2wsh in this context makes restricting p2pk
...

💬 jonatack commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1296427115)
Yes, that is currently an issue as well that I have noticed. See also https://github.com/bitcoin/bitcoin/pull/28155 (which I plan to review).

💬 LarryRuane commented on pull request "test: rpc: add last block announcement time to getpeerinfo result":
(https://github.com/bitcoin/bitcoin/pull/27052#issuecomment-1681276711)
Force-pushed to implement review suggestion to use `TicksSinceEpoch()` (thanks!)

💬 Retropex commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1681346103)
@Russeree you seem obstinate in believing that I'm trying to force things, that's not the case.

I am also not looking for the consensus that will come naturally or not depending on the relevance of the changes.

I just want to have tools to give nodes runner the choice. #27589

Unlike @petertodd who tried to remove a tool for nodes runner #28130, I want others to be implemented.

📝 andrewtoth opened a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280)
Since https://github.com/bitcoin/bitcoin/pull/17487 we no longer need to clear the coins cache when syncing to disk. A warm coins cache significantly speeds up block connection, and only needs to be fully flushed when nearing the `dbcache` limit.

For frequent pruning flushes there's no need to empty the cache and kill connect block speed. However, simply using `Sync` in place of `Flush` actually slows down a pruned full IBD with a high `dbcache` value. This is because as the cache grows, sync
...

💬 russeree commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1681361730)
@Retropex

> @russeree you seem obstinate in believing that I'm trying to force things, that's not the case.

Sorry for misinterpreting your statement this is what threw me off...

> Unfortunately despite the fact that this is not an isolated request (almost) no developer has helped us on the subject.

conveys a sense of bias or at least a perspective of disappointment. The term "unfortunately" connotes a negative view on the situation, implying that the outcome or response (or lack the
...

💬 fjahr commented on pull request "lint: fix custom mypy cache dir setting":
(https://github.com/bitcoin/bitcoin/pull/28184#discussion_r1296511440)
That was the previous behavior. There is even an explicit line for this in our `.gitignore`. What would you like to do instead? Move it into `/test/cache/`?

💬 fjahr commented on pull request "lint: fix custom mypy cache dir setting":
(https://github.com/bitcoin/bitcoin/pull/28184#discussion_r1296516711)
This was added as part of #18210 but I didn't see an explanation given there. I think it is better to specify a location explicitly since the default is using the current directory which may be leading to unexpected results if this script is called from different contexts. But if we don't care about something like this, it may be removed.

💬 sipa commented on pull request "Descriptor unit tests and simplifications":
(https://github.com/bitcoin/bitcoin/pull/24361#issuecomment-1681381767)
Leaving this as up for grabs. I still think it's a good idea, but don't have time to pursue it now.

✅ sipa closed a pull request: "Descriptor unit tests and simplifications"
(https://github.com/bitcoin/bitcoin/pull/24361)

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1296524385)
If these were iterators into the map, rather than pointers to the value, I think you wouldn't need the `m_outpoint`.

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1296525115)
Done.

💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1296525550)
Actually just the second condition suffices (the entire expected message is moved to the transport). I've simplified the code accordingly.