💬 achow101 commented on pull request "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase":
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2229392087)
done

💬 achow101 commented on pull request "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase":
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2229393691)
I prefer to leave this as-is as it is a more specific check. It checks for the db types that are allowed to be legacy wallets, rather than allowing legacy wallets for all non-sqlite databases.

💬 achow101 commented on pull request "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase":
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2229393890)
Removed now that there's `MockableSQLiteDatabase`

🤔 ryanofsky reviewed a pull request: "ipc: add bitcoin-mine test program"
(https://github.com/bitcoin/bitcoin/pull/30437#pullrequestreview-3052852068)
Thanks for the reviews!

Updated 2910315d1d8a3879f4e0bd50bf90870b642bfede -> 9d8851c0103f0dff8d659950e48d70ec86c86b30 ([`pr/mine.27`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.27) -> [`pr/mine.28`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.28), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.27..pr/mine.28)) just updating two code comments.
Updated 9d8851c0103f0dff8d659950e48d70ec86c86b30 -> 3d099384a60c8f64c8d67a9f1e7b8649a9c54313 ([`pr/mine.28`](https://
...

💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2229307480)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2213158587

> nit:

Thanks, updated!

💬 ryanofsky commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2229351651)
re: https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2213947741

> This should also break when the chain tip changes or generate a new template. Right now, it's possible to pass a large `max_tries` and get stuck.

I agree this is not ideal, though I don't think it's necessarily a problem. Probably more ideally this code would use two threads and increment the nonce in one thread and call waitNext in another thread, updating the hash if the tip changes. Calling getTip() every time th
...

💬 maflcko commented on pull request "rpc: use CScheduler for relocking wallet and remove RPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3114706601)
Just wanted to leave a note that this uncovers a false positive libc++ tsan warning, if anyone runs into this.

<details><summary>more details</summary>

https://cirrus-ci.com/task/5230471308640256?logs=check#L1679:

```
WARNING: ThreadSanitizer: data race (pid=22900)
Write of size 8 at 0x720c00021ab0 by thread T1 (mutexes: write M0):
#0 operator delete(void*, unsigned long) <null> (bitcoind+0x17e6cc) (BuildId: cfbd7d2fe965bb79ec7e44f4d6b230691518e5cd)
#1 void std::__1::__lib
...

🤔 brunoerg reviewed a pull request: "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase"
(https://github.com/bitcoin/bitcoin/pull/33032#pullrequestreview-3053056674)
Concept ACK

💬 maflcko commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2229432505)

calling call block_template->waitNext() -> calling block_template->waitNext() [duplicate “call” makes the phrase awkward and potentially confusing]

🤔 brunoerg reviewed a pull request: "test: Add functional tests for blockreconstructionextratxn parameter"
(https://github.com/bitcoin/bitcoin/pull/33023#pullrequestreview-3053070561)
I think the description is not clear about what you're trying to cover/achieve with the test. Can you explain it better? Also, corecheck doesn't show any related new coverage.

💬 maflcko commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2229441917)
I see. Though, it may still be good to make this one `explicit` to avoid silent uint256 conversions in the future?

Also, `Txid::data` is `std::byte*`, so `value_type` should probably be `std::byte` as well.

I haven't tried, but I presume both modifications should compile with afe99b3b7df6c658fb4068c6418dfd7ebf151460

👍 brunoerg approved a pull request: "wallet: Remove `upgradewallet` RPC"
(https://github.com/bitcoin/bitcoin/pull/32944#pullrequestreview-3053079846)
Concept & light cr ACK d89c6fa4a71810cdb28395d4609632e1b22249b3

💬 glozow commented on pull request "[29.x] final changes for v29.1rc1":
(https://github.com/bitcoin/bitcoin/pull/33056#issuecomment-3114741444)
thanks! done

💬 glozow commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2229453544)
Comment above this declaration needs to be changed

💬 glozow commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2229461893)
Hm, is there a reason why `m_tx_inventory_known_filter` should contain sometimes-txids instead of wtxids?

👍 brunoerg approved a pull request: "doc: add note for watch-only wallet migration"
(https://github.com/bitcoin/bitcoin/pull/32866#pullrequestreview-3053110010)
ACK 9d25880bb720bc675a533098268b9e02f86e17ce

Happy to re-ack if you touch it again to address the LLM linter suggestion.

💬 maflcko commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#issuecomment-3114779229)
[18:13:33.790] ./wallet/test/coinselector_tests.cpp(1247): [1;31;49merror: in "coinselector_tests/srd_tests": check EquivalentResult(expected_result, *res) has failed [0;39;49m

https://api.cirrus-ci.com/v1/task/6251945985310720/logs/ci.log

💬 bigshiny90 commented on pull request "test: Add functional tests for blockreconstructionextratxn parameter":
(https://github.com/bitcoin/bitcoin/pull/33023#issuecomment-3114783342)
> I think the description is not clear about what you're trying to cover/achieve with the test. Can you explain it better? Also, corecheck doesn't show any related new coverage.

This tests the -blockreconstructionextratxn parameter extra pool memory used specifically for compact block reconstruction. The "extra transaction pool" stores transactions that were rejected from the mempool for policy reasons (dust, low fees, non-standard scripts). Also, rbf original transactions get stuffed in here
...

💬 w0xlt commented on pull request "[POC] wallet: Enable non-electronic (paper-based) wallet backup with codex32":
(https://github.com/bitcoin/bitcoin/pull/33043#issuecomment-3114785492)
The emphasis of this PR is on backup functionality: a backup feature isn’t very useful unless you can also restore the wallet reliably.

By contrast, the other PRs aim only to import a Codex32 secret—presumably from Core Lightning into Bitcoin Core—and each uses a different method.
The approach here creates a fresh wallet and rescans the chain, whereas PR  #32652 appears to convert a Codex32 secret into an xpub via the proposed `addhdkey` RPC.

💬 martinatime commented on issue "getbestblockhash is sometimes taking a very long time":
(https://github.com/bitcoin/bitcoin/issues/32733#issuecomment-3114802238)
My RPi 4 only has 4GB of RAM and I am only running off of the SSD with no SD card installed.