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

📝 pinheadmz opened a pull request: "system: cache config file path before potentially updating datadir"
(https://github.com/bitcoin/bitcoin/pull/27303)
Fixes an issue where a `bitcoin.conf` file that contains a `datadir=` setting would be incorrectly reported in the logs (see examples below). This fix is implemented by caching the path to the conf file before setting `datadir`.

## Master

```
Default data directory /Users/matthewzipkin/Library/Application Support/Bitcoin
Using data directory /tmp/bcdata/regtest
Config file: /tmp/bcdata/bitcoin.conf (not found, skipping)
Config file arg: blocksdir="/tmp/blocks"
Config file arg: datadir
...

💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1145326660)
2fa935b389ac05c35 As `getaddrmaninfo` would be a new RPC, this code would break `-addrinfo` for clients using this code to call to long-running nodes running earlier versions of bitcoind starting from v22 (#21595). Please maintain compatibility (not in output, just the call still working) with these versions in your approach.

💬 TheCharlatan commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1145331263)
> If there isn't some more convenient way to review changes like this, I would definitely appreciate it if the code could just be moved and not reformatted.

ACK, I will stop using `clang-format-diff` on all moved code. Thank you for the review!

💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1145335359)
`CLI -addrinfo previously only returned a subset of addresses` and referring to a hidden RPC might raise more questions than it answers here for users trying to figure out what's going on.

Maybe borrow from the current -addrinfo help, along the lines of `CLI -addrinfo previously returned addresses known to the node after filtering for quality and recency. It now returns all of the addresses known to the node.`

💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1480193552)
> ping @jonatack, could you have a look, especially at the changes to bitcoin-cli?

Thanks! Sorry for missing the ping and not looking sooner.

💬 jonatack commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1145385658)
Testing the output of this RPC and -addrinfo side-by-side to check their output, I found myself mentally summing the RPC values to compare with the -addrinfo totals and thinking that it would be handy to have the sum returned by the RPC as well, i.e. new/tried/total.

```suggestion
"\nReturns the number of addresses in the `new` and `tried` tables and their sum for all networks (default) or a given network. This RPC is for testing only.\n",
```

```
$ ./src/bitcoin-c
...

💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1145355201)
Unimplemented function

💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1145368125)
Strictly speaking, the descendant size can only be greater than the ancestor size.

```suggestion
Assume(descendant->second.vsize_with_ancestors > anc->second.GetTxSize());
```

💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1145373839)
As the ancestors structure is a set and we are merely looping over it, I don't see how this can realistically happen. Two different iterators pointing to the same object? (if that is the worry, then the problem is with the iterators usage approach)

💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1145394065)
What is the reason behind this separate removal loop? Couldn't we merge it with the loop that is above?

Are you worry about the `ancestors` set having a parent and a child so if first removes the child then the parent will not find it and then crash?

💬 willcl-ark commented on issue "contrib/install_db4.sh script fails on OpenBSD 7.2 (RPi 3) (error: Unable to find a mutex implementation)":
(https://github.com/bitcoin/bitcoin/issues/26860#issuecomment-1480275742)
@m-kubik were you able to get this working with depends? If so I think we can probably close this issue.

💬 willcl-ark commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1145426398)
I think I'd lean here towards option 3, not breaking backwards-compatibility and just let json2.0 users "suffer" a `status: 500, error: -32700` :)

📝 ishaanam opened a pull request: "wallet: track mempool conflicts with wallet transactions"
(https://github.com/bitcoin/bitcoin/pull/27307)
The `TxStateMempoolConflicted` class is added. It is serialized the same as `TxStateInactive` and `TxStateInMempool`. This is done because mempool-related states are relatively short lived, so it doesn't make sense to serialize them differently than they are on master (mempool-conflicted txs are currently marked as `TxStateInactive`). As a result, this PR only changes how these transaction are dealt with in memory.

In memory `TxStateConflicted` (which only refers to block conflicts) and `TxSt
...