💬 jnewbery commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1146327740)
Style nit: feel free to use a single line for single-line if blocks:

```c++
if (complete) node.MarkReceivedMsgsForProcessing(nReceiveFloodSize);
```

💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1146502076)
That's clever! They generate similar code, but this first way is slightly less code https://godbolt.org/z/Pz77TTEd8
```
int f(int bytes)
{
return bytes > 0 ? (bytes+8-1)/8 : 1;
}
```

Or https://godbolt.org/z/adzasoPad
```
int f(int bytes)
{
return (bytes+8-1)/8 + (bytes == 0);
}
```
That's using x86-64 gcc 11.3; results are similar for x86-64 clang 16.0.0.

✅ rebroad closed a pull request: "Correct poor grammar in wallet synchronization warning."
(https://github.com/bitcoin-core/gui/pull/720)

👋 MarcoFalke's pull request is ready for review: "test: getblock on header throws"
(https://github.com/bitcoin/bitcoin/pull/27237)

🚀 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)