👍 tdb3 approved a pull request: "test: Fix intermittent failure in p2p_v2_misbehaving.py"
(https://github.com/bitcoin/bitcoin/pull/30420#pullrequestreview-2168907582)
ACK 9cb68fa7dcd286629d727abce3282ce8a33d8acc

Tested with the `sleep()` statement added into `generate_keypair_and_garbage()` in master and the PR branch. Failure only occurred on master (as expected).

💬 maflcko commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#issuecomment-2220406069)
Concept ACK, because errors and warnings should be extremely rare to occur. If anyone is interested in them, and looking for them in the debug log, they will be interested in warnings, if they are looking for errors (and vice-versa). So making a distinction here (and spending effort on it) seems possibly confusing without a clear upside.

It should be clear that an alert is fatal, if the program is shutting down (or aborting) as a result of it. Similarly, it should be clear that an alert is a
...

🤔 maflcko reviewed a pull request: "chainparams: Change nChainTx type to uint64_t"
(https://github.com/bitcoin/bitcoin/pull/29656#pullrequestreview-2168916547)
ACK 242116cdf3fac6fa9af1d01387cca2cf4be1ccb 🕕

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 242116cdf3fac6fa9af1d01387c
...

💬 maflcko commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#discussion_r1672161985)
nit in e78cf83beec2f508823803947bdc3d9bcb7c7217: Not sure why nbits matter for this test.

I think you can just write `CBlockIndex block{.nChainTx=..max()};`, replacing the first two lines.

💬 naiyoma commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#issuecomment-2220412410)
tested ACK [https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6](https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6), built and tested locally, test passes successfully.

💬 maflcko commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#issuecomment-2220417110)
> Additionally this modernizes the name of `nChainTx` to `m_chain_num_tx` which helps reviewers check all use of the symbol and can make silent merge conflicts.

I think you can drop the new name from the pull description. It is wrong right now (doesn't match the scripted diff), and if anyone wanted to find it, they can look at the scripted diff.

💬 maflcko commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#issuecomment-2220434793)
> > Can we get a test for this? Fixes are great but tests are awesome to not break it again in the future.
>
> Our own test framework was behaving that way before #29353, so maybe the functional tests would have caught it? I thought about reverting [c340503](https://github.com/bitcoin/bitcoin/commit/c340503b67585b631909584f1acaa327f5af25d3) locally to find out, but didn't have the time yet.

It could make sense to add one peer connection in the functional tests where the peer aggressively s
...

👍 alfonsoromanz approved a pull request: "test, assumeutxo: Remove resolved todo comments and add new test"
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2169033870)
Re ACK e523bcfc3403700debe1f286fa8c3db2d347e40b

💬 alfonsoromanz commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1672233485)
**Nit:** Missing a "be", i.e. "which should *be* the normal case." Not sure if it's worth fixing, but just wanted to point it out

💬 hebasto commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2220484016)
Concept ACK.

👍 andrewtoth approved a pull request: "rpc, rest: Improve getblock error when only header is available"
(https://github.com/bitcoin/bitcoin/pull/30410#pullrequestreview-2169081215)
Concept ACK

💬 andrewtoth commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1672260218)
```suggestion
throw JSONRPCError(RPC_MISC_ERROR, "Undo not available (not fully downloaded)");
```

💬 andrewtoth commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1672265165)
It feels like the rule of the 3 is being hit here and this code could be DRY'd up? Maybe in a helper function that takes the string `"Block"` or `"Undo"` and does string formatting?

💬 andrewtoth commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1672259793)
```suggestion
throw JSONRPCError(RPC_MISC_ERROR, "Undo not available (pruned data)");
```

⚠️ rishkwal opened an issue: "utils: add support for `bitcoin-wallet` tool configuration file"
(https://github.com/bitcoin/bitcoin/issues/30421)
### Please describe the feature you'd like to see added.

I would like to propose the addition of support for a configuration file specifically for the `bitcoin-wallet` tool. This feature would allow users to specify command-line options in a configuration file (e.g., `wallet.conf`), similar to how `bitcoin.conf` works for `bitcoind`. The configuration file should be automatically loaded by `bitcoin-wallet` if present in the default data directory or specified via a command-line option.

### Is
...

💬 andrewtoth commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1672277667)
```suggestion
if (!(blockindex.nStatus & BLOCK_HAVE_UNDO)) {
```

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1672281518)
Ahh yes (this comment)[https://github.com/bitcoin/bitcoin/pull/28280/files#diff-f0ed73d62dae6ca28ebd3045e5fc0d5d02eaaacadb4c2a292985a3fbd7e1c77cL269-L270] is removed in this patch, and should actually probably go right here. Perhaps I do this in a follow-up to not invalidate your ACK?

💬 hebasto commented on pull request "refactor: modernize-use-equals-default":
(https://github.com/bitcoin/bitcoin/pull/30406#issuecomment-2220528557)
Concept ACK.

💬 stratospher commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#issuecomment-2220588439)
> I could verify that the issue I see with sleep(3) is fixed with the change here. However, when I increase the number from 3 to 5 or 10 I still see other errors, so there may be more potential for race conditions here.

@fjahr good find! both the errors which you shared are because of disconnections due to [`InactivityCheck` triggers](https://github.com/bitcoin/bitcoin/blob/9adebe145557ef410964dd48a02f3d239f488cd0/src/net.cpp#L1987) from setting `peertimeout=3` in the test. I've increased it
...

👍 ryanofsky approved a pull request: "util: Catch translation string errors at compile time"
(https://github.com/bitcoin/bitcoin/pull/30383#pullrequestreview-2169220669)
Code review ACK fa601ab9f7142cdb18c18c1128fc893cdffb3463