💬 jonatack commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253806859)
CI failing at this line.

https://cirrus-ci.com/task/5531648075235328?logs=ci#L3397
https://cirrus-ci.com/task/5531648075235328?logs=ci#L4424

`Expected messages "['DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat']" does not partially match log:`

```
- 2023-07-05T18:23:01.237732Z [shutoff] [logging/timer.h:56] [Log] DumpAnchors: Flush 0 outbound block-relay-only peer addresses to anchors.dat started
- 2023-07-05T18:23:01.286842Z [shutoff] [logging/timer.h
...

💬 jonatack commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253818208)
Reproduced this test failure locally on the 4th run of `test/functional/feature_anchors.py` (arm64 macOS 13.4.1)

💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1253827052)
We already require `NET_*` to go from 0 to NET_MAX with no gaps every time we do `for (int n = 0; n < NET_MAX; ++n)`. It's a standard expectation of enums and it's documented as such in netaddress.h. It's not an added requirement, and doesn't make the code fragile or harder to change...

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1253855939)
reverted to the original optional arg in the latest push (https://github.com/bitcoin/bitcoin/compare/7a1c663e3727695a081a735c37819241341f3730..1e65aae8068ecf313a7c3b176dfc1326b3b67fbe) - checking for `preferred_net` meant I changed the default behavior. in order to fix that I would have had to add another else statement which seems more confusing than the original implementation, so went back

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1622876973)
small change to revert the function signature `MaybePickPreferredNetwork` to have network be an optional. can see https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1253855939 if you're interested in more detail.

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1253898394)
In commit "make GetFirstStoredBlock assert that 'start_block' always has data" (aad83e489a9f515a1da36acd3e99f35b1a0da53c)

The `pruneblockchain` RPC doesn't consider it an error to prune a block that's already been pruned. It just makes a best effort to prune what it can, and then it returns the last height of block without transaction data.

So I think it would be more consistent to not make this an error, and just do:

```c++
PruneBlockFilesManual(active_chainstate, height);
return blo
...

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1253906502)
In commit "make GetFirstStoredBlock assert that 'start_block' always has data" (aad83e489a9f515a1da36acd3e99f35b1a0da53c)

This preserves current behavior, but it is inconsistent with the documentation which says the field is be set to "height of the last block pruned, plus one". To make the code actually consistent with the documentation, this should be:

```c++
obj.pushKV("pruneheight", has_tip_data ? chainman.m_blockman.GetFirstStoredBlock(tip)->nHeight : tip.nHeight + 1);
```

Altern
...

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1253746784)
In commit "make GetFirstStoredBlock assert that 'start_block' always has data" (aad83e489a9f515a1da36acd3e99f35b1a0da53c)

It seems like there is a slight change in behavior here. Previously if the tip did not have data, but all blocks before it did have data there would not be a prune violation. Now there will be prune violation.

This is probably a good thing. I don't think this case actually needs to be an error if the missing blocks are <= than the assumeutxo snapshot height, and the blo
...

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1253746547)
In commit "make GetFirstStoredBlock assert that 'start_block' always has data" (aad83e489a9f515a1da36acd3e99f35b1a0da53c)

re: https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248314125

> I think that the changes are minimal and help to reduce confusion. But let me know what you guys think.

Thanks for that. I think it would have been fine to just keep the `GetFirstStoredBlock` behavior unchanged, and just document it, but fixing the call sites is probably better in the long run
...

💬 S3RK commented on pull request "wallet: Give deprecation warning when loading a legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/27869#issuecomment-1623103601)
reACK 8fbb6e99bfc85a1b9003cae402a7335843a86abd

💬 MarcoFalke commented on pull request "test: bugfix, synchronize indexes synchronously":
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1623118058)
> I mean, I'm also ok if we simply rise the timeout but, at the same time, I don't think that the background threads creation adds any meaningful test coverage here.

Right, it doesn't add any meaningful coverage. Though, I think the main point is that we should try to avoid adding test-only code to "real" code where possible. I am fine with anything, though.

🤔 S3RK reviewed a pull request: "Bump unconfirmed ancestor transactions to target feerate"
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1512188451)
Continue tests review

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251575760)
nit: since you create new wallet for every test and never reuse any addresses do you really need to generate blocks? I tried to delete and the tests pass

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251588782)
nit: here and other tests add `self.assert_spends_only_parent`

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251589066)
nit: add `self.assert_spends_only_parent`

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1254037039)
nit: why there is a multiplier here?

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1251595920)
Here and other tests.I'm not sure the magic number is needed. I tried setting the multiplier to 1 and the tests still pass. Maybe just check that resulting fee rate is exactly equal to target fee rate?

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1254028212)
This could be extended to a set of parent txs to verify cases with multiple parents

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1254048054)
nit: sffo is not needed

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1254044504)
Isn't `test_preset_input_cpfp` testing this?

⚠️ megumin9 opened an issue: "The destructor of CCheckQueueControl may throw a exception "
(https://github.com/bitcoin/bitcoin/issues/28033)
In src/checkqueue.h, the destructor of CCheckQueueControl will call Wait, which may throw a condition_error exception in 'void condition_variable::wait(unique_lock<mutex>& m)'. This will cause a crash, Is it intended? If not, the exception from Wait can be caught as follows to avoid the crash.

if (!fDone)
Wait();
if (pqueue != nullptr) {
LEAVE_CRITICAL_SECTION(pqueue->ControlMutex);
}

--->

try {
if (!fDone)
Wait();
if (pqueue != nullptr) {
LEAVE_CRITICAL_SECTION(
...