💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567792369)
Note that the argument to `ignore()` is unsigned (a `size_t`), might want to error out here if somehow `index < pos`.
(same for the occurence in `InternalPage`)

💬 achow101 commented on pull request "Wallet: Add GetFullBalance(...) which included used balance":
(https://github.com/bitcoin/bitcoin/pull/29062#discussion_r1567793472)
This doesn't need to be a separate function, just have `GetBalance` include `m_mine_used`. It's up to the caller to figure out what to do with that information.

💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567799862)
It looks like `fclose()` does nothing if `db_file.IsNull()` already holds.

💬 achow101 commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1567805211)
In 8a2021c0f4f40f6f8a37dd5619ecfa3f38084679 "tests: assert xpub order in sortedmulti descriptors do not matter"

It's not clear to me what this `random.sample` is supposed to do or how it improves the test.

💬 achow101 commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1567795960)
In 80ad8690e25c1ecab2ab1ff282882d5a6745f496 "tests: improve wallet multisig descriptor test and docs"

I don't think it's necessary to explain where the descriptor came from, just a description of it is fine.

💬 maflcko commented on issue "Intermittent issue in test/ipc_tests.cpp Fatal glibc error: pthread_mutex_lock.c:450 (__pthread_mutex_lock_full): assertion failed: e != ESRCH || !robust":
(https://github.com/bitcoin/bitcoin/issues/29889#issuecomment-2059743863)
Actually, I think the logs are complete for the log level `test_suite`. If more details are needed `test_suite` could be changed to `all`.

💬 maflcko commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1567825773)
`wait_for_debug_log` (the variant of the function that accepts raw bytes) does not have a `sleep`, so it will try to maxx out IO at 100%, no?

💬 achow101 commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1567828103)
In 858fa78637041ae704005d4b6e564dd8245f4b5d "test: Handle functional test disk-full error"

The `+`s at the beginning of the lines are unecessary.

```suggestion
f"Test execution data left in {tmpdir}.\n"
f"Additional storage is needed to execute testing.")
```

💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567831243)
Could add a check that `is_key` is true at the end. If not, there's an odd number of values, which would be odd?

💬 russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1567835969)
https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1239035489

The choice to do this was a response to the above comment. I would agree chainparams would be the place to implement a hardcoded (char[], stringview) prefix. Another advantage is that lookup is likely faster; so if a user is frequently getting errors it may save a few them a few cycles.

💬 maflcko commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#discussion_r1567840431)
> say, when you forget to include bitcoin-config)

In theory, there is a linter to check this. However, the linter could be tricked by guarding the include by `#if defined(HAVE_C0NFIG_H)` (`O` replaced by `0`). This is fixed in #29494.

💬 Krokochik commented on issue "importdescriptors hanging on importing/updating descriptor with large range":
(https://github.com/bitcoin/bitcoin/issues/25895#issuecomment-2059776680)
@willcl-ark no it doesn't

💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567845152)
Instead of casting back and forth and having a `GetRealType()` it would, imo, be more straightforward to split the delete flag off from the type in `Unserialize` before casting to `RecordType`. So to have
```
RecordType type;
bool deleted;
```
Then these accessors could go.

🤔 achow101 reviewed a pull request: "tests: add functional test for miniscript decaying multisig"
(https://github.com/bitcoin/bitcoin/pull/29156#pullrequestreview-2004420996)
e0b0a385350d8f7a9d00542cc3d14bb47224d889 "tests: miniscript decaying multisig signing order does not matter" could be squashed into de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig".

💬 achow101 commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1567831190)
In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

It's not necessary to state where the descriptor comes from.

💬 achow101 commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1567833329)
In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

Using multiple wallets is preferred over using multiple nodes.

💬 achow101 commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1567838489)
In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

A lot of this is very similar to/the same as `wallet_multisig_descriptor_psbt.py`. I would prefer if they were combined into one test file. Note that you can (and should) make separate functions for the different test cases so that there is a distinct separation between them, while still using the same utility functions.

💬 achow101 commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1567843544)
In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

Mining hundreds of blocks can take a while and is unnecessary for this test. This could run quite a bit faster with locktimes that are closer to each other.

💬 achow101 commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1567842740)
In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

This workflow is very similar to what will happen in the loop below. Can they be consolidated?

💬 achow101 commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1567846617)
In de93b863583921929bc16feb7a0bf5decfbc7ac5 "tests: add functional test for miniscript decaying multisig"

This massively overshoots the locktime block each time. I think it would be better to mine just enough blocks to reach the locktime height.