💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1569053476)
> 🐙 This pull request conflicts with the target branch and [needs rebase](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes).

🕺

💬 mzumsande commented on pull request "test: fix intermittent error in getblockfrompeer.py":
(https://github.com/bitcoin/bitcoin/pull/27784#issuecomment-1569054690)
> Can the original be reproduced by running the test in a loop??

I haven't been able to reproduce it, I think it requires a constellation with a very special timing which should make it very rare even in the CI. One thing I did is check in the log is that node2 doesn't use `HeadersDirectFetchBlocks` for any blocks > 205, and uses compact block reconstruction instead. This is different in the log of the failed run.

💬 theStack commented on pull request "test: fix intermittent error in getblockfrompeer.py":
(https://github.com/bitcoin/bitcoin/pull/27784#issuecomment-1569062154)
> [9fe9074](https://github.com/bitcoin/bitcoin/commit/9fe9074266c6d7ddb40384d81741df1fc94980ff) looks harmless, but I haven't delved into the underlying issue. Can the original be reproduced by running the test in a loop??

If you want to give that approach a try, the following shell script (which I have been running for days already, without any issue) should do exactly that:
```bash
#!/usr/bin/env bash
set -e
while true; do ./test/functional/rpc_getblockfrompeer.py; done
```

💬 erikarvstedt commented on issue "bitcoind hangs waiting for `g_requests.empty()`":
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1569074534)
@Crypt-iQ, the slow progress of `importmulti` is indeed a separate issue. I've updated my post.

💬 willcl-ark commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1210781561)
Import helper from _test_framework.messages_?

```suggestion
new_data_hash = hash256(new_data)

```

💬 willcl-ark commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1210795850)
Why do we need to keep the `Socks5Server` alive to ensure success when performing `"Check for addrv2 addresses in anchors.dat"`?

I changed this to `False` and saw intermittent failures on Line 123

```
assert_equal(data[services_index], 0x00) # services == NONE
```

💬 willcl-ark commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1210781782)
```suggestion
from test_framework.messages import CAddress, hash256
```

📝 furszy opened a pull request: "fuzz: fix wallet notifications.cpp"
(https://github.com/bitcoin/bitcoin/pull/27786)
Fixing https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1568815816.

As the fuzzer test requires all blocks to be scanned by the wallet
(because it is asserting the wallet balance at the end), we need to
ensure that no blocks are skipped by the recently added wallet
birth time functionality.

This just means setting the chain accumulated time to the maximum
value, so the wallet birth time is always below it, and the block is
always processed by the wallet.

💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1569086614)
Thanks Marco. Fixed in #27786.
Would be nice if the CI alert us about this kind of stuff prior merging the PR.
The base fuzzer corpus should have detected it right?.

⚠️ Macoophonic opened an issue: "Adchoose"
(https://github.com/bitcoin/bitcoin/issues/27787)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

thuannguyen19992000@gmail.com

### Expected behaviour

Eppo

### Steps to reproduce

It idon's data no way of the world

### Relevant log output

......

### How did you obtain Bitcoin Core

Package manager

### What version of Bitcoin Core are you using?

Đọc.join.29.v

### Operating system and version

13.0.

### Machine specifications

_No response_

✅ willcl-ark closed an issue: "Adchoose"
(https://github.com/bitcoin/bitcoin/issues/27787)

:lock: fanquake locked an issue: "Adchoose"
(https://github.com/bitcoin/bitcoin/issues/27787)

👍 ryanofsky approved a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1442091917)
Code review ACK 3c51e44380ba8041e5d6c4cb29b9b2c54fad0b4b

Overall changes look good, but I think two things could be done to make this easier to review:

1. The main thing would be making the "walletdb: refactor X loading" commits self contained and not leave behind dead code that needs to be cleaned up later. It's hard to identify what code is being replaced in current commits when not all the code being replaced is actually removed.
2. I think it might be good to move first 2 commits up t
...

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1204298801)
In commit "wallet: Add GetPrefixCursor to DatabaseBatch" (422a8436089c844934903a61fcec6b7b93995c07)

If prefix being non-empty is a requirement, it would be good to add this `Assume` to the `MockableBatch` cursor as well, so all of the implementations of this method consistently reject empty prefixes, and test cases don't appear to work with the mock database and then fail with real databases.

Alternately you could consider just moving the `Assume` checks out of the GetNewPrefixCursor metho
...

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1204326544)
In commit "walletdb: Refactor key reading and loading to its own function" (82e025297ba8939660b15fad738be723d783165e)

Note for other reviewers: There appears to be a slight change of behavior because now `wss.nKeys` will be incremented if the public key is not valid. This doesn't actually change behavior because counts are not used if there are any failures:

https://github.com/bitcoin/bitcoin/blob/214f8f18b310e3af88eba6a005439ae423ccd76a/src/wallet/walletdb.cpp#L932-L933

Also `wss` vari
...

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1204957277)
In commit "walletdb: Refactor hd chain loading to its own function" (f079b7580d4e7fa0d373decd1ffd7144abb83bab)

Seems ok, but is there a rationale for previous commits catching exceptions in the refactored functions and this commit not catching exceptions?

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1205489696)
In commit "salvage: Remove use of ReadKeyValue in salvage" (e9d974b7f4136c04f35091438f5e78e17d77299b)

I guess this will no longer catch std::exception, but that is ok?

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1206093314)
In commit "walletdb: refactor tx loading" (ad3ae8b6f4b2f3430b9d6c1b5bcf2627be15d961)

It's confusing that this commit leaves behind dead code implementing the same logic as the new code. Would suggest getting rid of it:

```diff
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -300,11 +300,8 @@ bool WalletBatch::EraseLockedUTXO(const COutPoint& output)

class CWalletScanState {
public:
- bool fAnyUnordered{false};
- std::vector<uint256> vWalletUpgrade;
st
...

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1205886280)
In commit "walletdb: Refactor legacy wallet record loading into its own function" (b3425cc31feba14c5a0b0c4532437f12b5a901ff)

What its this change doing? It seems unrelated to the commit and maybe would be better as a separate commit with an explanation.

💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1206095209)
In commit "walletdb: Refactor descriptor wallet records loading" (51518daa60952f31c34013c643feb8fe11c9777e)

This commit is leaving behind dead code, including another copy of this print statement. Would be better to get rid of it here:

```diff
--- a/src/wallet/walletdb.cpp
+++ b/src/wallet/walletdb.cpp
@@ -310,11 +310,7 @@ public:
std::vector<uint256> vWalletUpgrade;
std::map<OutputType, uint256> m_active_external_spks;
std::map<OutputType, uint256> m_active_internal_s
...