💬 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.

💬 achow101 commented on pull request "Simplify network-adjusted time warning logic":
(https://github.com/bitcoin/bitcoin/pull/29623#issuecomment-2059790024)
reACK c6be144c4b774a03a8bcab5a165768cf81e9b06b

💬 pinheadmz commented on pull request "include verbose "debug-message" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#issuecomment-2059830018)
Thanks for taking a look, @0xB10C. I've refactored the PR to add a new field instead of changing anything already in place. Edited PR title and description. I made the tests super clear that the complete string is returned by `submitrawtransaction` whereas its split into two fields in `testmempoolaccept`

💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1567887865)
OK I see now thanks. So I guess... `start_node` calls `wait_for_rpc_connection`. Is RPC available before reindexing is finished? If we can rely on that to prevent race conditions then this could just be `assert_debug_log`.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567920748)
Good point. Removed this check.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567920893)
Added a check

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567920972)
Removed

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567921413)
Good point. I've added a check earlier that the number of records is even to avoid this kind of issue.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567921595)
Done as suggested.