💬 achow101 commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1480012074)
It turns out that encrypting a blank legacy wallet also does not unset the blank flag nor does it generate a new seed.

The latest push removes the descriptor wallet changes and adds tests for the behavior when encrypting.

🚀 fanquake merged a pull request: "test: Remove unused Check* default constructors"
(https://github.com/bitcoin/bitcoin/pull/27297)

💬 fanquake commented on pull request "ci: Use TSan new runtime (llvm-16, take 3)":
(https://github.com/bitcoin/bitcoin/pull/27298#issuecomment-1480024555)
Looks like just explicitly installing `libclang-rt-16-dev` will solve this. This branch works for me, for running the TSAN CI job on x86_64: https://github.com/fanquake/bitcoin/tree/27298_plus_libclang:
```bash
ALL | ✓ Passed | 3493 s (accumulated)
Runtime: 991 s

Stop and remove CI container by ID
48aee56b02c33313b240f21c0b9924c68bfa9fcaaa16d0557adcd31b1efb73be

real 32m28.067s
```

Still testing on aarch64.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1480027322)
> Have you looked into a fuzz target for this parser?

I haven't. If someone would like to write one, I'd be happy to add it.

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1145223127)
Hm yeah good point, a few error codes like this [are specified](https://www.jsonrpc.org/specification#error_object). One problem with this is that our application error `RPC_INVALID_PARAMETER` (`-8`) is coded ~186 times in RPC functions everywhere. I suppose we could re-map those error codes back to the jsonrpc 2.0 spec in `JSONRPCExecOne()`? Only when the request indicates jsonrpc 2.0

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1145229348)
In the tests at least it was originally `version: "1.1"` as seen in [authproxy.py](https://github.com/bitcoin/bitcoin/pull/27101/files#diff-6e2d6b181408a11452bcf83c79b299ba195524165e8a46e746b9d712961cc6fb) which is actually [in the 1.1 spec](https://www.jsonrpc.org/historical/json-rpc-1-1-wd.html#ProcedureCall). I'm not sure if `"jsonrpc": "1.0"` was part of an older spec or if thats a mistake [here](https://github.com/bitcoin/bitcoin/pull/27101/files#diff-fc0f86b6c3bb23b0e983bcf79d7546d1c9eaa15
...

💬 fanquake commented on pull request "ci: Use TSan new runtime (llvm-16, take 3)":
(https://github.com/bitcoin/bitcoin/pull/27298#issuecomment-1480041089)
Ran into a failure (with the above branch, and NO_BDB=1), on aarch64:
```bash
2023-03-22T18:02:11.462544Z (mocktime: 2020-08-31T15:34:12Z) [txindex] [util/thread.cpp:20] [TraceThread] txindex thread start
test/txindex_tests.cpp(38): fatal error: in "txindex_tests/txindex_initial_sync": critical check time_start + timeout_ms > GetTimeMillis() has failed
LLVMSymbolizer: error reading file: No such file or directory
make[3]: Leaving directory '/home/fedora/ci_scratch/ci/scratch/build/bitcoin-a
...

💬 ryanofsky commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1145230536)
In commit "refactor: Extract util/fs_helpers from util/system" (59c02e68f75f7b727162d2ea36bf3ed99fabf035)

Anybody have advice on easiest way to review changes like this when blocks of code are moving but also changing slightly at the same time?

I find it pretty easy to review blocks of moved code in git with the `--color-moved` option, but once code is moved and reformatted at the same time (the `{` at the end of this line was previously on its own line) it becomes very tedious to find the
...

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1145232671)
aw geez. Well, ok so it looks like `disconnectnode` is the only RPC that throws the (correct?) error code for invalid params, while the rest throw `RPC_INVALID_PARAMETER` (`-8`). Maybe it does make sense to use a scripted diff to replace all the `-8`s with `-32602`. Nice catch!

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1145235420)
Not sure if `RPC_INTERNAL_ERROR` (`-32603`) is totally correct for estimating fee from a blocksonly node. It's more of a bad request ...

💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1145236261)
After having a closer look, you are right that currently allocating & freeing 0 bytes does not work with the resource. But I think I can't allow allocating 0 bytes with the resource at all, because when allocating multple 0 bytes it would always give out the same pointer, and the standard says that this is not allowed: https://cplusplus.github.io/LWG/issue9

So I think the best solution is to just add a `bytes > 0` condition to `IsFreeListUsable`, so that `::operator new` will be used in that
...

💬 achow101 commented on pull request "bumpfee: allow send coins back to yourself":
(https://github.com/bitcoin/bitcoin/pull/27195#issuecomment-1480050513)
ACK c260d3fbf4cf8be07176840f39125abcb3d59c1b

👍 ryanofsky approved a pull request: "refactor: Extract util/fs from util/system"
(https://github.com/bitcoin/bitcoin/pull/27254)
Code review ACK 3ceb8dde48fc12f4f16372f661a4cccf7104e194

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1145246067)
This error is caused by a failure to parse JSON in `ReadBody()` so theoretically we also can't read the `"jsonrpc": "2.0"` pair either. I agree with you though, the HTTP transport is fine and should be 200, because the error is coming from the json parser. I guess the options are:
1. always send 200 [here](https://github.com/bitcoin/bitcoin/blob/master/src/httprpc.cpp#L77) and kinda break backwards compat
2. attempt to parse the `"jsonrpc"` string and only send 200 if we can detect the "2.0" v
...

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1145250554)
yep thanks, will update

👍 john-moffett approved a pull request: "refactor: Replace GetTimeMicros by SystemClock"
(https://github.com/bitcoin/bitcoin/pull/27233)
ACK faf3f12424fa8558e65fa3f1dd3aa1d0eea8604e changes, but left a comment for the existing code.

💬 john-moffett commented on pull request "refactor: Replace GetTimeMicros by SystemClock":
(https://github.com/bitcoin/bitcoin/pull/27233#discussion_r1145186659)
Very unlikely, but could this result in undefined behavior? `FormatISO8601DateTime` can theoretically return an empty string if it's fed, eg, an extremely large value and `gmtime` fails. While I can't see any reason for that happening (the libstdc++ `::max()` values I checked are reasonable) maybe best to avoid it just in case?

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1145263307)
another good call, thats a bug. I force-pushed with a fix and added a mixed 1.1 + 2.0 in the batch test

👋 brunoerg's pull request is ready for review: "test: add support for all networks in `deserialize_v2`"
(https://github.com/bitcoin/bitcoin/pull/27295)

💬 brunoerg commented on pull request "test: add support for all networks in `deserialize_v2`":
(https://github.com/bitcoin/bitcoin/pull/27295#issuecomment-1480091589)
@fanquake That error seems unrelated to this PR