💬 hodlinator commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1974305941)
I just realized the PR description shouldn't contain the @-symbol. See:

https://github.com/bitcoin/bitcoin/blob/3c1f72a36700271c7c1293383549c3be29f28edb/CONTRIBUTING.md?plain=1#L170-L174

💬 Prabhat1308 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1974334806)
Removed the @ symbol

💬 Prabhat1308 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1974391971)
Added the notes under the commands.

💬 jonatack commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#issuecomment-2689246685)
Concept ACK, appears to be a standalone version of [`189eb8c` in #31250](https://github.com/bitcoin/bitcoin/pull/31250/commits/189eb8c02b5b81fe3239df34cd9ef48871e2e5de)

⚠️ Crypt-iQ opened an issue: "compact blocks in IBD resets m_stalling_since"
(https://github.com/bitcoin/bitcoin/issues/31962)
During IBD it is possible that a malicious peer sends unsolicited compact-block messages to reset its `m_stalling_since` state. This requires two or more malicious peer connections and can happen with the following steps:
- Malicious peer A sends a headers message that contains height `h+1`. The node requests block `h+1` from peer A.
- Malicious peer B sends a headers message that contains heights `{h+1, h+2, ..., h+1024}`. The node requests `MAX_BLOCKS_IN_TRANSIT_PER_PEER` blocks from B. Peer B
...

🤔 maflcko reviewed a pull request: "Require sqlite when building the wallet"
(https://github.com/bitcoin/bitcoin/pull/31961#pullrequestreview-2650014687)


There are a few leftovers. Genereally, when removing stuff, you can use git grep to find leftovers: `git grep WITH_SQLITE`. This also makes me wonder why CI passed. I would have expected for depends builds a warning:

```
CMake Warning:
Manually-specified variables were not used by the project:
WITH_SQLITE
```

💬 maflcko commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#discussion_r1974967547)
duplicate,unrelated change?

💬 maflcko commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#issuecomment-2689990649)
Otherwise:

review ACK 26b588534f4f9ec15a8a10a423a5ba120c290428 🏜

<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: review AC
...

💬 maflcko commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1975006000)
Why are you restricting this to 100? It wasn't restricted before

💬 maflcko commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1975006163)
same

💬 maflcko commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r1975028703)
Missing `reject_reason` field?

💬 hodlinator commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2690303014)
Ran into this linker error when attempting to test #31161 on Windows yesterday.
Thanks for working on this, good to see linker warnings as errors enabled in CI!

The kernel library exposing `cs_main` instead of exposing functions that automatically take the correct locks makes me want to cry. But we have to deal with where we are.

Concept ACK.

💬 maflcko commented on pull request "descriptors: inference process, do not return unparsable multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31404#issuecomment-2690353123)
```
[17:25:46.471] test_framework.authproxy.JSONRPCException: Internal bug detected: RPC call "decoderawtransaction" returned incorrect type:
[17:25:46.471] {
[17:25:46.471] "vout": {
[17:25:46.471] "1": {
[17:25:46.471] "scriptPubKey": {
[17:25:46.471]
...

💬 maflcko commented on pull request "init: Take lock on blocks directory in BlockManager ctor":
(https://github.com/bitcoin/bitcoin/pull/31860#issuecomment-2690448271)
> Supporting multiple wallets in the same directory is mentioned there, but it is not said why that might be a desirable feature.

I haven't tested this, but it may have been required in the past, because BDB wallets may just be files, not folders, so they could all sit in the same walletdir (which could even be the datadir)? If true, this requirement may go away with dropping bdb?

🤔 rkrux reviewed a pull request: "test: Fix authproxy named args debug logging"
(https://github.com/bitcoin/bitcoin/pull/31955#pullrequestreview-2650606826)
> In Python the meaning of args or argsn is that argsn is fully ignored when args is a list with at least one element.

The following code prints `argsn` fine in this scenario.

#### Code
```
def f(name, *args, **argsn):
print(name)
print(args)
print(argsn)

f("titlehere", [1,2], a=10, b=30)
```

#### Output
```
titlehere
([1, 2],)
{'a': 10, 'b': 30}
```

I don't think it's a generic Python thing. I believe `argsn` not being logged was due to the following older
...

💬 maflcko commented on pull request "test: Fix authproxy named args debug logging":
(https://github.com/bitcoin/bitcoin/pull/31955#issuecomment-2690608447)
@rkrux Not sure if I understand your question, but if you used `args or argsn` in your print: `print(args or argsn)`, then `argsn` is omitted from the print if the `args` list has at least one element. This should be true in general for all versions of Python that are supported. The changes in this pull request are fixing the problem, as explained in the pull request description.

💬 rkrux commented on pull request "test: Fix authproxy named args debug logging":
(https://github.com/bitcoin/bitcoin/pull/31955#issuecomment-2690634285)
Yes, correct, I agree. The PR description statement I highlighted in my comment led me to believe that `argsn` is ignored **_always in any function_** if `args` is a list with at least 1 element, which I found to be confusing to read and hence verified it with an example. The issue as I understand is with the usage of `args or argsn` as the first argument to the `json.dumps` function, which this PR correctly fixes.

The only concern is with the first statement in the PR description, which is t
...

💬 maflcko commented on pull request "test: Fix authproxy named args debug logging":
(https://github.com/bitcoin/bitcoin/pull/31955#issuecomment-2690649864)
There may be a formatting issue. For me the pull description says (with code blocks):

> In Python the meaning of `args or argsn` is that `argsn` is fully ignored when `args` is a list with at least one element.

However, for you it seems to say:

> In Python the meaning of args or argsn is that argsn is fully ignored when args is a list with at least one element.

Obviously, the two have different meaning (and the second doesn't really make sense). I am not sure how to fix this. What cl
...

💬 rkrux commented on pull request "test: Fix authproxy named args debug logging":
(https://github.com/bitcoin/bitcoin/pull/31955#issuecomment-2690711672)
Oh interesting, I see where the confusion is. I do see the same description as you but I didn't read that particular part as a code expression, which made it sound too generic to me. Never mind.

💬 Sjors commented on pull request "Require sqlite when building the wallet":
(https://github.com/bitcoin/bitcoin/pull/31961#issuecomment-2690746991)
@maflcko I checked for `USE_SQLITE` but not `WITH_SQLITE`. Hunted down the latter.