💬 fanquake commented on pull request "p2p: remove adjusted time":
(https://github.com/bitcoin/bitcoin/pull/25908#issuecomment-1481401163)
Rebased on master. @ajtowns putting aside the current comments, any thoughts on including changes like this in bitcoin-inquisition?

💬 TheCharlatan commented on pull request "compat: move (win) S_* defines into bdb":
(https://github.com/bitcoin/bitcoin/pull/26832#issuecomment-1481403430)
Light code review ACK cf0d86ed15041822e2a0a55ddf231af7809a9e66

👋 hebasto's pull request is ready for review: "clang-tidy: Add more `performance-*` checks and related fixes"
(https://github.com/bitcoin/bitcoin/pull/26642)

💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1146387794)
Yes, `FundTransaction` currently throws away `nLockTime` thereby disabling anti-fee-sniping, and this preserves that behavior.

💬 hebasto commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1481425866)
> Show an error on startup if a bitcoin datadir that is being used contains a `bitcoin.conf` file that is ignored.

Concept ACK on that.

💬 hebasto commented on pull request "clang-tidy: Add more `performance-*` checks and related fixes":
(https://github.com/bitcoin/bitcoin/pull/26642#issuecomment-1481428896)
> Seeing lots of https://cirrus-ci.com/task/6739422046060544?logs=ci#L339:

Sorry. Pushed a wrong branch.

Should be OK now.

✅ glozow closed an issue: "Plumb "too-long-mempool-chain" to RPC error for send/sendtoaddress"
(https://github.com/bitcoin/bitcoin/issues/23144)

🚀 glozow merged a pull request: "wallet: return error msg for "too-long-mempool-chain""
(https://github.com/bitcoin/bitcoin/pull/24845)

👍 ryanofsky approved a pull request: "refactor: rpc: remove ParseNonRFCJSONValue()"
(https://github.com/bitcoin/bitcoin/pull/27256)
Code review ACK 860e70713c2c4a13f9cb108046669ad9fdb02331. This looks good, but we will have to find another function to give the most confusing function name award to. I left a few minor review suggestions, but they can be ignored.

💬 ryanofsky commented on pull request "refactor: rpc: remove ParseNonRFCJSONValue()":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146386553)
In commit "test: move coverage on ParseNonRFCJSONValue() to UniValue::read()" (6e28dedcd6fe44b84ddfac71f3325305e5219885)

I don't think you should delete these two lines entirely, because doing that really seems to lose test coverage for the `AmountFromValue` function despite the claim in the commit description. I would keep this two lines, but move them to the previous test and replace `ParseNonRFCJSONValue` with `ValueFromString`.

💬 ryanofsky commented on pull request "refactor: rpc: remove ParseNonRFCJSONValue()":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146414393)
In commit "refactor: rpc: remove ParseNonRFCJSONValue() function" (860e70713c2c4a13f9cb108046669ad9fdb02331)

This commit description seems misleading (or maybe it is out of date?). The commit is not removing the `ParseNonRFCJSONValue()` function, just renaming it and making it private.

I think a better name for the commit would be something like "Hide and rename ParseNonRFCJSONValue()", and a better name for the PR would be "Remove unnecessary uses of ParseNonRFCJSONValue() and rename it".
...

💬 ryanofsky commented on pull request "refactor: rpc: remove ParseNonRFCJSONValue()":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146389196)
In commit "test: move coverage on ParseNonRFCJSONValue() to UniValue::read()" (6e28dedcd6fe44b84ddfac71f3325305e5219885)

Test order seems to have changed unintentionally. Would be good to keep the 19e-6 test before the 1e+30 for easier comparison.

💬 ryanofsky commented on pull request "refactor: rpc: remove ParseNonRFCJSONValue()":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146418012)
In commit "refactor: rpc: remove ParseNonRFCJSONValue() function" (860e70713c2c4a13f9cb108046669ad9fdb02331)

Probably should add `static` to avoid exporting a linker symbol that could clash with a `Parse(string_view)` function in another translation unit.

💬 willcl-ark commented on issue "Running RPC call importmulti with a xpub key with very large range results in a timeout error":
(https://github.com/bitcoin/bitcoin/issues/17797#issuecomment-1481495736)
@chris-belcher have you tried this with the new native descriptor wallet from [April 2020](https://github.com/bitcoin/bitcoin/pull/16528)? I have just tried to reproduce* and importing the range of 999,999 took 1m25s (and therefore didn't trigger the http timeout you experienced):

```fish
will@ubuntu in ~/.bitcoin
₿ /home/will/src/bitcoin/src/bitcoin-cli createwallet "test-wallet-disable-privkeys" true
{
"name": "test-wallet-disable-privkeys",
"warning": ""
}

will@ubuntu in ~/.bi
...

💬 jnewbery commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#issuecomment-1481496992)
utACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e

I didn't verify myself that the new code doesn't result in a copy of `CNetMessage`, but I trust that you've tested that. A follow up PR could potentially delete the default copy constructor for `CNetMessage` to ensure that we never copy.

All of my review comments are nits/style comments and shouldn't stop this from being merged.

💬 stickies-v commented on pull request "refactor: rpc: remove ParseNonRFCJSONValue()":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146452144)
> BOOST_CHECK_THROW(AmountFromValue(ParseNonRFCJSONValue(".19e-6")), std::runtime_error);

Unless I misunderstand, `AmountFromValue` is not even called because `ParseNonRFCJSONValue` already throws first, so I don't think we lose any coverage here? And the happy path is already tested at the moment:

```
BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.19e-6")), 19);
```
> BOOST_CHECK_EQUAL(AmountFromValue(ParseNonRFCJSONValue("0.00000000000000000000000000000000000001e+30 ")), 1);
...

💬 vincenzopalazzo commented on issue "core stops to run with `Failed to read block` error":
(https://github.com/bitcoin/bitcoin/issues/27142#issuecomment-1481499943)
@willcl-ark after the downgrade to the tagged version I have not try it again, sorry!

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

```
Traceback (most recent call last):
File "C:\Users\ContainerAdministrator\AppData\Local\Tem
...

💬 stickies-v commented on pull request "refactor: rpc: remove ParseNonRFCJSONValue()":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146457650)
The change is intentional, I'm now first doing the valid tests followed by the one invalid test, making it consistent with the docs too. I think this is easier to understand? But I don't care too much either way, happy to change the order (and add some more docs) if you think that makes it easier.

💬 stickies-v commented on pull request "refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146469854)
Thanks, I've adopted your phrasing for both commit and PR description.