🚀 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

📝 achow101 converted_to_draft a pull request: "bumpfee: Allow the user to choose which output is change"
(https://github.com/bitcoin/bitcoin/pull/26467)
When bumping the transaction fee, we will try to find the change output of the transaction in order to have an output whose value we can modify so that we can meet the target feerate. However we do not always find the change output which can cause us to unnecessarily add an additional output to the transaction. We can avoid this issue by outsourcing the determination of change to the user if they so desire.

This PR adds a `orig_change_pos` option to bumpfee which the user can use to specify t
...