🚀 fanquake merged a pull request: "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg"
(https://github.com/bitcoin/bitcoin/pull/27257)

👍 ryanofsky approved a pull request: "refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it"
(https://github.com/bitcoin/bitcoin/pull/27256)
Code review ACK 79ee14bf1ea9530922e5d15edd35f9debf661998. Just added `static` and updated commit description since last review

💬 ryanofsky commented on pull request "refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146540762)
re: https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146452144

> ```c++
> BOOST_CHECK_THROW(AmountFromValue(ParseNonRFCJSONValue(".19e-6")), std::runtime_error);
> ```

Wow, it didn't occur to me that AmountFromValue call on this line was useless because it was bypassed by the exception. I'm not sure if the person who wrote this test originally knew the AmountFromValue was being skipped, but it definitely looks to any reasonable observer like this line is intending to check whet
...

💬 ryanofsky commented on pull request "refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146548353)
Makes sense, and fine if this is intentional, I just thought it was an accident.

💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1146560809)
The code generation is a bit different when you use unsigned types, then the `+ (bytes == 0)` version is the shortedst for me. In my microbenchmark the `+ (bytes == 0)` is also fastest, but in practice its most likely irrelevant

💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1146611376)
re: https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1146473010

> Whilst I appreciate that this is a simple and elegant way to remain backwards-compatible, I can't help but wonder if it's something we actually want to support with a new option; what is the use-case here? The operator either has to modify their current config with this new flag, or... fix their config?

There may be other use cases, but the backwards compatibility use-case I had in mind is where someone has a bitcoi
...

💬 achow101 commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#issuecomment-1481673419)
ACK fe367f2e4e5fb95546fa26491d43346e0bc11adb

💬 stickies-v commented on pull request "refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it":
(https://github.com/bitcoin/bitcoin/pull/27256#issuecomment-1481691410)
Force pushed to [add test a test case on `AmountFromValue()`](https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146540762) that was previously not reached but probably intended.

Addressed all of @ryanofsky's comments - thank you for the review!

⚠️ ryanofsky opened an issue: "wallet_create_tx.py "Not solvable pre-selected input" exception"
(https://github.com/bitcoin/bitcoin/issues/27316)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

CI test failure https://cirrus-ci.com/task/5315879898972160?logs=functional_tests#L2675:

```
test 2023-03-23T17:23:41.645000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
self.run_test()
File "C:\Users\Con
...

💬 stickies-v commented on pull request "refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146645081)
Thanks, you're probably right that it was intended to be covered like that so I've added your test case on `.19e-6`.

💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1146653864)
I'll implement the ` + (bytes == 0)` for NumElemAlignBytes

💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1481707292)
Updated 53d99551e913bfb85769a2b34fed73898779ff0f -> 972335762aeaab557dbb2e44b149b18005987f8b ([`pr/ignoredconf.5`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.5) -> [`pr/ignoredconf.6`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ignoredconf.5..pr/ignoredconf.6?w=1)) to try to fix another CI error on win64:

```
TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "C:\U
...

👍 ryanofsky approved a pull request: "refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it"
(https://github.com/bitcoin/bitcoin/pull/27256)
Code review ACK cfbc8a623b5133f1d0b0c0c9be73b2b107e0d687. Only change since last review is adding a new test

📝 john-moffett opened a pull request: "log: Check that the timestamp string is non-empty to avoid undefined behavior"
(https://github.com/bitcoin/bitcoin/pull/27317)
Follow-up to https://github.com/bitcoin/bitcoin/pull/27233

The current `FormatISO8601DateTime` function will return an empty string if it encounters an error when converting the `int64_t` seconds-since-epoch to a formatted date time. In the unlikely case that happens, here `strStamped.pop_back()` would be undefined behavior.

💬 MarcoFalke commented on pull request "test: getblock on header throws":
(https://github.com/bitcoin/bitcoin/pull/27237#issuecomment-1481769276)
Not sure if it makes sense to add a test only. Maybe the error message can be improved along? Closing, let's continue discussion in #https://github.com/bitcoin/bitcoin/issues/20978

✅ MarcoFalke closed a pull request: "test: getblock on header throws"
(https://github.com/bitcoin/bitcoin/pull/27237)

💬 MarcoFalke commented on pull request "log: Check that the timestamp string is non-empty to avoid undefined behavior":
(https://github.com/bitcoin/bitcoin/pull/27317#discussion_r1146718782)
This should never happen, so instead of adding dead code, what about simply combining the two if conditions?

💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1481782854)
Updated d87cb99 -> 9f947fc3d4b779f017332135323b34e8f216f613 ([pr25325.1](https://github.com/martinus/bitcoin/commits/pr25325.1) -> [pr25325.2](https://github.com/martinus/bitcoin/commits/pr25325.2))

There is a single behavior change in pool.h, now `NumElemAlignBytes` adds `+ (bytes == 0)` so that allocations of 0 bytes work with the PoolAllocator.

Other than that, updated tests to include allocation of 0 bytes, and fixed all the nits.

💬 stratospher commented on pull request "p2p: Improve diversification of new connections":
(https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1481786231)
yes! thanks @mzumsande, will address this in a follow-up PR.

💬 john-moffett commented on pull request "log: Check that the timestamp string is non-empty to avoid undefined behavior":
(https://github.com/bitcoin/bitcoin/pull/27317#discussion_r1146738163)
My thinking was that if it never happens, then this PR is unnecessary. If it could happen, then I'd rather have an explicit message rather than output that entirely lacks timestamps, which someone may not notice is bizarre.

Happy to change it, though, if people prefer your approach.

📝 furszy opened a pull request: "test: wallet_create_tx.py fix race"
(https://github.com/bitcoin/bitcoin/pull/27318)
Fixes #27316

As wallets are internally synced by the
validation interface, and the interface
dispatches events on a worker thread,
it can happen that the tx sent from the
first wallet doesn't arrive to the second
wallet before the second wallet tries to
create a new transaction using one of
the outputs of the first tx as input.