💬 rserranon commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#issuecomment-1428851086)
testing ACK

👍 ponury1990 approved a pull request: "wallet: ensure the wallet is unlocked when needed for rescanning"
(https://github.com/bitcoin/bitcoin/pull/26347)

💬 w0xlt commented on pull request "[Draft / POC] Silent Payments":
(https://github.com/bitcoin/bitcoin/pull/24897#issuecomment-1429142222)
Update: The silentpayment index has been removed from the codebase.

`scantxoutset` has been changed to use `txindex` and `CBlockUndo`. So the scan works without silentpayment index.

`getsilentpaymentblockdata` also works without any indexes, using only `CBlockUndo`. The code is relatively simple. Therefore, even serving light clients, there is no need for indexing.

Seems to work fine. Not sure about the performance.

A new PR, with the index, will be added soon.

💬 darosior commented on pull request "Signing support for Miniscript Descriptors":
(https://github.com/bitcoin/bitcoin/pull/24149#discussion_r1105320169)
Will do if i need to retouch.

💬 martinus commented on pull request "Add simulation-based `CCoinsViewCache` fuzzer":
(https://github.com/bitcoin/bitcoin/pull/27011#issuecomment-1429201435)
@sipa While rebasing #25325 I saw that the fuzzer in `coins_view.cpp` still uses `CCoinsMap coins_map;` and doesn't use the deterministic seeds, shouldn't this fuzzer do that as well?

I can add it while rebasing, I'm touching that line anyways. Unless you want to do this separately

💬 hebasto commented on pull request "Check for the awk script before executing it":
(https://github.com/bitcoin/bitcoin/pull/27095#issuecomment-1429249881)
@Harshil-Jani

Thank you for your contribution! I think it is more appropriate to submit your patch to the [upstream project](https://github.com/kadwanev/retry).

💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1429293948)
Rebased due to #27011. Note that this adds a `/*deterministic=*/true` [here in test/fuzz/coins_view.cpp](https://github.com/bitcoin/bitcoin/pull/25325/commits/78f597be2879c39d9d2b98e21ed0120d2308de20#diff-1ef3b6a1936b50f3d5ec4a1786d9e2d63d1a3e1815b103e67f20601995f355b4R119), see `git range-diff 0007d69...78f597b`

💬 TheCharlatan commented on pull request "refactor, kernel: Remove gArgs accesses from dbwrapper and txdb":
(https://github.com/bitcoin/bitcoin/pull/25862#issuecomment-1429298769)
Code review ACK aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f

👍 TheCharlatan approved a pull request: "refactor: wallet, remove global 'ArgsManager' dependency"
(https://github.com/bitcoin/bitcoin/pull/26889)

💬 MarcoFalke commented on pull request "refactor: Move coin_control variable to test setup section":
(https://github.com/bitcoin/bitcoin/pull/26154#issuecomment-1429330974)
Closing for now due to controversy.

✅ MarcoFalke closed a pull request: "refactor: Move coin_control variable to test setup section"
(https://github.com/bitcoin/bitcoin/pull/26154)

💬 MarcoFalke commented on pull request "Use designated initializers":
(https://github.com/bitcoin/bitcoin/pull/24531#discussion_r1105484093)
Just for clarity and documentation. This code is wrong as-is and has long been removed. Omitting the field name here can lead to bugs, see https://godbolt.org/z/9TnoMKMT3 for a playground.

💬 hebasto commented on pull request "refactor: wallet, remove global 'ArgsManager' dependency":
(https://github.com/bitcoin/bitcoin/pull/26889#discussion_r1105535083)
Sorry for not being explicit in my previous comments. There are more cases where `uint64_t` could be replaced with `int64_t`:
```suggestion
int64_t m_keypool_size GUARDED_BY(cs_desc_man){DEFAULT_KEYPOOL_SIZE};
```

and parameter `keypool_size` in (member) functions.

💬 hebasto commented on pull request "build: Check usages of #if defined(...)":
(https://github.com/bitcoin/bitcoin/pull/25302#issuecomment-1429426979)
@brokenprogrammer
> I've squashed and updated the commit message according to the contributing guidelines.

A https://github.com/bitcoin/bitcoin/pull/25302#discussion_r1104616746 still needs to be addressed.

💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#issuecomment-1429434184)
> I will check this out, specifically w.r.t this comment which I had not been considering:

I don't believe this fully closes the loop on #16220, but I am also not sure that we ever will close that fully whilst also providing backwards-compatibility...

There can exist currently two different working wallet configurations:

1. Legacy: wallets in subfolders in top-level, e.g. `~/.bitcoin/test-wallet/wallet.dat` or `~/.bitcoin/regtest/test-wallet/wallet.dat`
2. Current: wallets in wallets/
...

💬 TheCharlatan commented on pull request "build: use _FORTIFY_SOURCE=3":
(https://github.com/bitcoin/bitcoin/pull/27027#issuecomment-1429435999)
Concept ACK

Ran this on Ubuntu 22.04 with glibc 2.35 and clang 14. Also did not observe a significant slowdown of the benchmarks.
On master:
```
objdump -d src/bitcoind | grep "_chk@plt" | wc -l
...
36
```
This branch:
```
objdump -d src/bitcoind | grep "_chk@plt" | wc -l
...
76
```

💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1105549095)
Thanks, I have updated to `EnsureDataDirNet` as suggested

💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1105552243)
Now updated to `EnsureDataDirNet` which always creates the `wallets/` path if it's creating a new DataDir, but otherwise doesn't.

💬 fanquake commented on pull request "Check for the awk script before executing it":
(https://github.com/bitcoin/bitcoin/pull/27095#issuecomment-1429472868)
Yep. Feel free to send this change upstream.

✅ fanquake closed a pull request: "Check for the awk script before executing it"
(https://github.com/bitcoin/bitcoin/pull/27095)