💬 russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-1628375439)
This PR has had some substantial updates applied since it's first inception.

1. Deterministically calculated base58 prefixes

2. This PR now greatly improves the accuracy of error reporting as seen in the updates to 3d0a5c37e9ccedfa4ecfaa48eeeca1ada5b4eec1 where many valid bech32 tests are fed through the base58 section of the `DecodeDestination` function. This line gives a great example of how this PR addresses this from the current
https://github.com/bitcoin/bitcoin/blob/79e8247ddb166f9
...

💬 S3RK commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1257851249)
v0.16 is different in many regards, does it make sense to have a separate test to check incompatibility?

💬 S3RK commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1257854317)
pull up the comment explaining why?

💬 fanquake commented on pull request "test: Ignore UTF-8 errors in assert_debug_log":
(https://github.com/bitcoin/bitcoin/pull/28035#issuecomment-1628449877)
> Is this fix applied to both for python2 and python3?

We only support Python 3.

💬 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-1628461546)
I tried on 25.0, where it will just fill the storage at full speed in a never ending loop (aborted after 30 minutes)


💬 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-1628476666)
```
2023-07-10T08:19:38Z [qt-rpcconsole] [wallet/scriptpubkeyman.cpp:1917] [MigrateToDescriptor] MigrateToDescriptor: not mine script: addr(mqRzyzrW7DPGT1KZN2mLuyGPVUugpfEVPW)#4ndsysze
bitcoin-qt: wallet/scriptpubkeyman.cpp:1919: std::optional<MigrationData> wallet::LegacyScriptPubKeyMan::MigrateToDescriptor(): Assertion `IsMine(desc_spk) != ISMINE_NO' failed.

💬 TheCharlatan commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1628492953)
Strong Concept ACK.

💬 MarcoFalke commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1628507151)
Would it make sense to document that this is expected to take a long time on spinning storage? For me a freshly created BDB wallet takes ~5 minutes to migrate. Another one I wasn't sure about, see https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1628461546.

Also, there seem to be outstanding bugs with the RPC, see same thread as above.

💬 MarcoFalke commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1628658906)
Thanks for the feedback so far. I've kept the option to disable, but made it a simple bool instead of allowing the user to pick the XOR-key.

If someone is interested, a new commit can be added to upgrade the linearize scripts to read the XOR-key.

💬 peterwillcn commented on issue "mandatory-script-verify-flag-failed (Public key is neither compressed or uncompressed)":
(https://github.com/bitcoin/bitcoin/issues/21098#issuecomment-1628662930)
from_addr: 1HT7xU2Ngenf7D4yocz2SAcnNLW7rK8d4E same issues

💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1258057941)
3 of the PRs are mine (I don't care about rebasing) and the package relay stuff is gonna conflict either way, so I'd prefer keeping it as is.

📝 MarcoFalke opened a pull request: " streams: Add XorFile "
(https://github.com/bitcoin/bitcoin/pull/28060)
This adds a new type, which has the `AutoFile` streams interface, but can roll an XOR pattern while reading or writing to the underlying file.

This is needed for https://github.com/bitcoin/bitcoin/pull/28052, but can also be used in any other place, where `AutoFile` can be used.

Also, there are tests, so I've split this up from the larger pull to make review easier, hopefully.

💬 MarcoFalke commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1628813020)
Also, I don't think this is a fix for https://github.com/bitcoin/bitcoin/pull/27444/files#r1165491936. This will still fail outside the CI. Here, you are only modifying the CI to use a different version of valgrind.

💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1628906716)
Hey there, I fixed the descriptions as suggested, and ran the tests and was able to build Bitcoin Core locally successfully.

💬 MarcoFalke commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1258239050)
Stray file?

💬 furszy commented on pull request "test: make assumeUTXO test capture the expected fatal error":
(https://github.com/bitcoin/bitcoin/pull/28050#issuecomment-1628936305)
> Also it may be good to name the commit that introduced the bug. Otherwise it will be harder for reviewers to see if there was a bug in that commit or if the change was intentional. Also, reviewers will be missing context.

Ok. It was introduced within the test 87a1108c8.
Still, this isn't as serious as you imagine. It is just an extra print. 100% sure that wasn't intended.

The test is exercising a specific fatal error (checking that is being triggered), and doing so it makes the node cal
...

💬 MarcoFalke commented on pull request "test: make assumeUTXO test capture the expected fatal error":
(https://github.com/bitcoin/bitcoin/pull/28050#issuecomment-1628940734)
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

💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1258250019)
Oh I'm sorry I must have forgotten to discard everything other than `mining.cpp`. Should I revert this commit, or something else?

🤔 MarcoFalke reviewed a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1521991201)
Looks like the tests fail

💬 MarcoFalke commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1258201606)
225e213110602b4fd1d345167f5f92d26557f6c1: Why ignore the return value? This makes the tests fail