💬 MarcoFalke commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1258195719)
18bf3abc2babbe066e5d4b5798209826316c8295: Why is this atomic, when only a single thread has access? Or is the other code multi thread safe?

💬 furszy commented on pull request "test: make assumeUTXO test capture the expected fatal error":
(https://github.com/bitcoin/bitcoin/pull/28050#issuecomment-1628956370)
> Are you sure, because locally I don't see it in 25.0:
>
> ```
> $ ./bitcoin-25.0/bin/test_bitcoin -t validation_chainstatemanager_tests/chainstatemanager_snapshot_completion_hash_mismatch
> Running 1 test case...
>
> *** No errors detected
> ```

Ok good, found the commit. 6eb33bd0c21b3e075fbab596351cacafdc947472 introduced it. It removed the `mock_shutdown` function from the test.

💬 furszy commented on pull request "test: make assumeUTXO test capture the expected fatal error":
(https://github.com/bitcoin/bitcoin/pull/28050#issuecomment-1628960452)
cc @TheCharlatan then. Thanks @MarcoFalke.

💬 MarcoFalke commented on pull request "test: make assumeUTXO test capture the expected fatal error":
(https://github.com/bitcoin/bitcoin/pull/28050#issuecomment-1628961804)
lgtm ACK 3e8bf2e10c26ef9c2f58307b523e4b674ac97a2c

💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1629006969)
@ryanofsky separate tests PR is ready for review: https://github.com/bitcoin/bitcoin/pull/27850

💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1258287264)
At the end of the PR, two threads end up accessing the variable. The `BaseIndex::Init` call stays in the main thread, while the `BaseIndex ::StartBackgroundSync` call is moved to the 'loadblk' thread (now called 'initload')

💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1258307033)
upps, fixed the intermediate commit. Thanks.

💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1258307306)
done

🤔 furszy reviewed a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1522158509)
Updated per feedback, thanks Marco and TheCharlatan.

Changes:
* Fixed bug in one of the intermediate commits (per [comment](https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1258201606)).
* Inlined an if/else in one of the intermediate commits (per [comment](https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1257458241)).

💬 MarcoFalke commented on pull request "test: Check expected_stderr after stop":
(https://github.com/bitcoin/bitcoin/pull/28028#discussion_r1258317110)
thx, done

💬 brunoerg commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#issuecomment-1629071257)
Concept ACK

👍 ryanofsky approved a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1522224733)
Code review ACK ca91c244ef1ba7eac6643d66a5fc56d3a2a8b550. Just rebase and suggested changes since last review (Start return check, and code simplification)

👍 TheCharlatan approved a pull request: "test: make assumeUTXO test capture the expected fatal error"
(https://github.com/bitcoin/bitcoin/pull/28050#pullrequestreview-1522225346)
ACK 3e8bf2e10c26ef9c2f58307b523e4b674ac97a2c

Thank you for fixing this.

💬 pinheadmz commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#issuecomment-1629092956)
@jonatack thank you sir for the great review and suggested patches, all nits addressed

💬 furszy commented on issue "migratewallet crashes (wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wallet::LegacyScriptPubKeyMan::MigrateToDescriptor(): Assertion `IsMine(desc_spk) != ISMINE_NO' failed.)":
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1629096788)
Could you provide the `getaddressinfo mqRzyzrW7DPGT1KZN2mLuyGPVUugpfEVPW` output please.

And also, do you have steps to reproduce it? how did you import that address?

💬 MarcoFalke commented on issue "migratewallet crashes (wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wallet::LegacyScriptPubKeyMan::MigrateToDescriptor(): Assertion `IsMine(desc_spk) != ISMINE_NO' failed.)":
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1629109069)
No idea where it is from. Apparently it is from a "test_2" :sweat_smile:

```
{
"address": "mqRzyzrW7DPGT1KZN2mLuyGPVUugpfEVPW",
"scriptPubKey": "76a9146cc16c24e249bab4384d4a759fe53ab23227de1988ac",
"ismine": false,
"solvable": false,
"iswatchonly": true,
"isscript": false,
"iswitness": false,
"ischange": false,
"timestamp": 0,
"labels": [
"test_2"
]
}

👍 ryanofsky approved a pull request: "test: Add unit & functional test coverage for blockstore"
(https://github.com/bitcoin/bitcoin/pull/27850#pullrequestreview-1522239261)
Code review ACK ca91c244ef1ba7eac6643d66a5fc56d3a2a8b550. The new tests seem useful and are very easy to read, and I think switching CI to run as non-root is a nice change that should make CI environment more useful and realistic.

I do think the instinct to skip tests is a mistake, though. Usually better to just let tests run in any environment and check whatever the expected behavior is in that environment, unless the behavior is really unstable or nondeterministic. In this case, rather than
...

💬 ryanofsky commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1258364600)
In commit "init: start indexes sync earlier" (ed4462cc78afd2065bbf5bd79728852b65b9ad7f)

Nice test. Blockman API is really confusing but the comments here make everything very clear, and having the test should make improvements easier to understand, and help ensure things don't get broken unintentionally.

⚠️ vasild opened an issue: "Auto detect IPv6 connectivity"
(https://github.com/bitcoin/bitcoin/issues/28061)
### Please describe the feature you'd like to see added.

Auto detect if the host has connectivity to the IPv6 network and only attempt to make automatic outbound ones if it has.

### Is your feature related to a problem, if so please describe it.

IPv6 is set to reachable by default. If it is not in practice then some outbound connection attempts will be made in vain, trying to connect to IPv6 hosts.

### Describe the solution you'd like

If the kernel does not have IPv6 support compiled in the
...

💬 fanquake commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1629153391)
> This will still fail outside the CI.

Yes, but outside the CI, the only route to solving that would be with more (achitecture specific) suppressions? If we don't control anything else, we can't make any guarantees about anything passing or not.

At least in this PR, it no-longer fails inside the CI, and that's difference between the CI working, and not, on aarch64.

Happy to followup with further improvements to add more architecture specific supressions or similar, for use outside the C
...