✅ maflcko closed a pull request: "refactor: Replace ALWAYS_FALSE with false"
(https://github.com/bitcoin/bitcoin/pull/29108)

💬 maflcko commented on pull request "refactor: Replace ALWAYS_FALSE with false":
(https://github.com/bitcoin/bitcoin/pull/29108#issuecomment-1860587423)
Oh nvm, this is C++26

⚠️ c0deright opened an issue: "Old wallet.dat: Error reading wallet database: keymeta found with unexpected path / All keys read correctly, but transaction data or address metadata may be missing or incorrect"
(https://github.com/bitcoin/bitcoin/issues/29109)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

When starting bitcoind with an old wallet (created July 2019, probably `v0.17.x` or `v0.18.x`) the log records 2 errors and one warning that are not self-explanatory.

Might be related to #19051

### Expected behaviour

No warning/errors are shown or the errors/warnings are more detailed as to what's going on.

### Steps to reproduce

Run v26.0 with a very old `wallet.dat` file. I wasn't
...

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1430198623)
I was just thinking on ways to simplify this. `CheckForStaleTipAndEvictPeers()` is executed every 45 seconds. I think it is ok if for a while (~ 10 mins) we think we are close to the tip but we are not in practice. Given that we realize this in a few minutes and don't get stuck with the wrong assumption forever.

👍 instagibbs approved a pull request: "wallet, mempool: propagete `checkChainLimits` error message to wallet"
(https://github.com/bitcoin/bitcoin/pull/28863#pullrequestreview-1787005073)
LGTM

💬 instagibbs commented on pull request "wallet, mempool: propagete `checkChainLimits` error message to wallet":
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1430210897)
this would be good

💬 ChrisMartl commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860650558)
Concept ACK

Database is a very important resource for the "Bitcoin"-System and its usage must be reserved exclusively for Sats endogenous monetary usage.
Since the Bitcoin's systemic flaw (https://github.com/bitcoin/bitcoin/issues/29089#issue-2043281653) is still unaddressed / unsolved, efforts to preserve the capability of already allocated storage devices resources in the network shall be considered and encouraged.

💬 ismaelsadeeq commented on pull request "wallet, mempool: propagete `checkChainLimits` error message to wallet":
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1430217862)
Thanks Would create a tiny follow-up to fix this, so as not to invalidate ACK's.

💬 brunoerg commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1860680045)
Concept ACK

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1430238465)
The problem this PR aims to resolve is that if we are too much behind (48h), then limited peers may not be able to give us the blocks we need. To resolve this problem, I think that it is not necessary to change the "extra block relay only connections" logic. IMO that better be done in a separate PR with its own justification.

💬 maflcko commented on pull request "doc: Clarify C++20 comments":
(https://github.com/bitcoin/bitcoin/pull/29042#issuecomment-1860707320)
Included https://github.com/bitcoin/bitcoin/pull/29108

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1430246659)
Fixed thanks

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1430246916)
fixed

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#issuecomment-1860731819)
Thanks for reviewing @luke-jr
Force pushed from https://github.com/bitcoin/bitcoin/commit/e032462c44dba9b6961c1e18bd597d63c6afac02 to https://github.com/bitcoin/bitcoin/commit/e032462c44dba9b6961c1e18bd597d63c6afac02.

[Compare diff](https://github.com/bitcoin/bitcoin/compare/e032462c44dba9b6961c1e18bd597d63c6afac02..814571bf3ebb4e2640bbb6401c5822c038bb5287)

The changes in latest push are:
- Rebased on master to fix CI
- Updated the non-standardness reason to address @luke-jr comments

...

💬 fanquake commented on issue "ci: feature_proxy failing in MSVC job":
(https://github.com/bitcoin/bitcoin/issues/29090#issuecomment-1860741967)
Similar failure, different test: https://github.com/bitcoin/bitcoin/actions/runs/7249043694/job/19746211856#step:27:521.

⚠️ fanquake opened an issue: "ci: failure in `wallet_basic.py --descriptors`"
(https://github.com/bitcoin/bitcoin/issues/29110)
https://cirrus-ci.com/task/5898005006516224?logs=ci#L2515:
```bash
_20231218_134952/wallet_basic_259/node0/regtest/wallets/default_wallet/wallet.dat] SQLite Statement: ROLLBACK TRANSACTION
node0 2023-12-18T13:53:29.193915Z [scheduler] [wallet/sqlite.cpp:400] [Close] SQLiteBatch: Batch closed unexpectedly without the transaction being explicitly committed or aborted
node0 2023-12-18T13:53:29.194038Z [scheduler] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_base/ci/scratch/test_
...

👍 ryanofsky approved a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-1782332723)
Code review ACK 5d5e8cbca785db851747c5143dc1d651a1ea11e6. This is definitely an improvement overall, but I think it leaves httprpc.cpp and rpc/server.cpp organization and the functional tests in a messier state, and I left suggestions for cleanup.

I was also a confused by parts of the PR description. I'm not sure what "different behavior for single and batched RPC requests" and "same behavior for single and batch requests" refers to, and think these parts could be omitted or clarified. Also "
...

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1427031318)
In commit "rpc: refactor single/batch requests" (fcc8cd08dbed9a44bab277ad267306dd2a9f5199)

Would be clearer if jreq were a const reference, since it is not modified

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1427085941)
In commit "MOVEONLY: define class before functions in request.h" (bcf90ae6b7fd4685755c3f2e1e736053e613e0f6)

It seems ok to move these functions, but for future reference, there should be no need to do this because you can forward declare the class with `class JSONRPCRequest;` at the top of the file. I do think it would be slightly preferable to do that instead of moving code, but it's not important.

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1427171338)
In commit "rpc: make error and result mutually exclusive in JSON-RPC 2.0 replies" (55dae852d88cdc6a4ad7e4a5cd3ba1988af9872c)

This comment is confusing because result and error _values_ are mutually exclusive in both cases.

You could clarify the comment saying "error and result _keys_ are mutually exclusive, to clarify that the comment is referring to keys not values. But I think the way this is written with duplicated logic is unnecessarily confusing anyway. Would suggest just writing the
...