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

💬 jonatack 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_r1146755842)
Assuming it can happen, maybe wrap the message in, say, square brackets ("[...]") to be clearer in the log.

💬 pinheadmz commented on issue "pruneblockchain should be able to increase the size of pruned blockchain":
(https://github.com/bitcoin/bitcoin/issues/24286#issuecomment-1481813132)
Ok, closing this for now as duplicate of https://github.com/bitcoin/bitcoin/issues/19807 which has a bit more active discussion. Will try to close that one as well in favor of #21267 if OP agrees. @prusnak please feel free to comment if you think we need to keep this one open as well.

✅ pinheadmz closed an issue: "pruneblockchain should be able to increase the size of pruned blockchain"
(https://github.com/bitcoin/bitcoin/issues/24286)