The behavior is not new, but this rule exits earlier than before.
Previously, a carve out could have been granted in PreChecks() but then
nullified in PackageMempoolChecks() when CheckPackageLimits() is called
with the default limits.
cbc6c440e3 doc: add comments and release-notes for JSON-RPC 2.0 (Matthew Zipkin)
e7ee80dcf2 rpc: JSON-RPC 2.0 should not respond to "notifications" (Matthew Zipkin)
bf1a1f1662 rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests (Matthew Zipkin)
466b90562f rpc: Add "jsonrpc" field and drop null "result"/"error" fields (Matthew Zipkin)
2ca1460ae3 rpc: identify JSON-RPC 2.0 requests (Matthew Zipkin)
a64a2b77e0 rpc: refactor single/batch requests (Matthew Zipkin)
df6e3756d6 rpc: Avoid copies in JSONRPCReplyObj() (Matthew Zipkin)
09416f9ec4 test: cover JSONRPC 2.0 requests, batches, and notifications (Matthew Zipkin)
4202c170da test: refactor interface_rpc.py (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/2960
Bitcoin Core's JSONRPC server behaves with a special blend of 1.0, 1.1 and 2.0 behaviors. This introduces compliance issues with more strict clients. There are the major misbehaviors that I found:
- returning non-200 HTTP codes for RPC errors like "Method not found" (this is not a server error or an HTTP error)
- returning both `"error"` and `"result"` fields together in a response object.
- different error-handling behavior for single and batched RPC requests (batches contain errors in the response but single requests will actually throw HTTP errors)
https://github.com/bitcoin/bitcoin/pull/15495 added regression tests after a discussion in https://github.com/bitcoin/bitcoin/pull/15381 to kinda lock in our RPC behavior to preserve backwards compatibility.
https://github.com/bitcoin/bitcoin/pull/12435 was an attempt to allow strict 2.0 compliance behind a flag, but was abandoned.
The approach in this PR is not strict and preserves backwards compatibility in a familiar bitcoin-y way: all old behavior is preserved, but new rules are applied to clients that opt in. One of the rules in the [JSON RPC 2.0 spec](https://www.jsonrpc.org/specification#request_object) is that the kv pair `"jsonrpc": "2.0"` must be present in the request. Well, let's just use that to trigger strict 2.0 behavior! When that kv pair is included in a request object, the [response will adhere to strict JSON-RPC 2.0 rules](https://www.jsonrpc.org/specification#response_object), essentially:
- always return HTTP 200 "OK" unless there really is a server error or malformed request
- either return `"error"` OR `"result"` but never both
- same behavior for single and batch requests
If this is merged next steps can be:
- Refactor bitcoin-cli to always use strict 2.0
- Refactor the python test framework to always use strict 2.0 for everything
- Begin deprecation process for 1.0/1.1 behavior (?)
If we can one day remove the old 1.0/1.1 behavior we can clean up the rpc code quite a bit.
ACKs for top commit:
cbergqvist:
re ACK cbc6c440e3
ryanofsky:
Code review ACK cbc6c440e3. Just suggested changes since the last review: changing uncaught exception error code from PARSE_ERROR to MISC_ERROR, renaming a few things, and adding comments.
tdb3:
re ACK for cbc6c440e3
Tree-SHA512: 0b702ed32368b34b29ad570d090951a7aeb56e3b0f2baf745bd32fdc58ef68fee6b0b8fad901f1ca42573ed714b150303829cddad4a34ca7ad847350feeedb36
42fb5311b1 rpc: return warnings as an array instead of just a single one (stickies-v)
Pull request description:
The RPC documentation for `getblockchaininfo`, `getmininginfo` and `getnetworkinfo` states that "warnings" returns "any network and blockchain warnings". In practice, only a single warning (i.e. the latest one that is set) is returned, the other ones are ignored.
Fix that by returning all warnings as an array.
As a side benefit, clean up the GetWarnings() logic.
Since this PR changes the RPC result schema, I've added release notes. Users can temporarily revert to the old results by using `-deprecatedrpc=warnings`, until it's removed in a future version.
---
Some historical context from git log:
- when `GetWarnings` was introduced in 401926283a, it was used in the `getinfo` RPC, where only a [single error/warning was returned](401926283a (diff-7442c48d42cd5455a79915a0f00cce5e13359db46437a32b812876edb0a5ccddR250)) (similar to how it is now).
- later on, "warnings" RPC response fields were introduced, e.g. in ef2a3de25c, with the description [stating](ef2a3de25c (diff-1021bd3c74415ad9719bd764ad6ca35af5dfb33b1cd863c0be49bdf52518af54R411)) that it returned "any network warnings" but in practice still only a single warning was returned
ACKs for top commit:
achow101:
re-ACK 42fb5311b1
tdb3:
Re ACK for 42fb5311b1
TheCharlatan:
ACK 42fb5311b1
maflcko:
ACK 42fb5311b1🔺
Tree-SHA512: 4225ed8979cd5f030dec785a80e7452a041ad5703445da79d2906ada983ed0bbe7b15889d663d75aae4a77d92e302c93e93eca185c7bd47c9cce29e12f752bd3
5195baa600 depends: fix miniupnpc snprintf usage on Windows (fanquake)
3c2d440f14 depends: switch miniupnpc to CMake (Cory Fields)
f5618c79d9 depends: add upstream CMake patch to miniupnpc (fanquake)
6866b571ab depends: miniupnpc 2.2.7 (fanquake)
Pull request description:
This picks up one of the changes from #29232, which is a switch to building miniupnpc with CMake. It includes an update to the most recent version of miniupnpc (2.2.7), which means we can drop one patch from that commit, and includes a new patch for a change I've upstreamed https://github.com/miniupnp/miniupnp/pull/721, as well as some suggestions from the previous PR.
ACKs for top commit:
theuni:
ACK 5195baa600.
TheCharlatan:
utACK 5195baa600
Tree-SHA512: 5b27e132cd5eed285e9be34c8b96893417d92a1ae55c99345c9a89e1c1c5e40e4bc840bc061b879758b2b11fcb520cd98c3da985c1e153f2e5380cf63efe2d69
The RPC documentation for `getblockchaininfo`, `getmininginfo` and
`getnetworkinfo` states that "warnings" returns "any network and
blockchain warnings". In practice, only a single warning is returned.
Fix that by returning all warnings as an array.
As a side benefit, cleans up the GetWarnings() logic.
1ea8674316 [doc] update release-process.md and backports section of CONTRIBUTING (glozow)
Pull request description:
While doing various release process things for the first time, I noticed some of our docs are outdated and/or confusing.
ACKs for top commit:
achow101:
ACK 1ea8674316
Tree-SHA512: 4ad10d4ce2c33fe15cb02599353107bb72ecb867aefc6c120cfd5cdea42aa8fa3783f9e0218c2f3815f030e0694cc8fb24011ce88358a0206cb07416a256a962
2179e2c320 doc: i2p: improve `-i2pacceptincoming` mention (brunoerg)
Pull request description:
In i2p documentation, it says that "the first time Bitcoin Core connects to the I2P router,
it automatically generates a persistent I2P address and its corresponding private key by
default _**or if `-i2pacceptincoming=1` is set**_". This is weird, because `-i2pacceptincoming=1`
by itself does not have any effect. Moreover, `-i2pacceptincoming` is 1 by default anyway.
ACKs for top commit:
laanwj:
This documentation change is correct and makes the documentation slightly shorter, thus easier to read. ACK 2179e2c320
davidgumberg:
ACK 2179e2c320
achow101:
ACK 2179e2c320
byaye:
ACK 2179e2c320
Tree-SHA512: 18a6a627343fb0aa824029d99df8a232153ba288ce94ec8c5da25693885237381fba505ea1e71c756b2a611243a302d319ca7ae03b526020cd6588710fc2ac17
3c1ae3ee33 depends: switch libnatpmp to CMake (Cory Fields)
72ba7b5d26 depends: libnatpmp f2433bec24ca3d3f22a8a7840728a3ac177f94ba (fanquake)
Pull request description:
This picks up one of the changes from https://github.com/bitcoin/bitcoin/pull/29232, which is a switch to building libnatpmp with CMake. It includes an update to the most recent version of libnatpmp (f2433bec24), which includes (https://github.com/miniupnp/libnatpmp/pull/43).
From an initial look I couldn't find any significant difference between the Autotools and CMake produced libs.
ACKs for top commit:
m3dwards:
ACK 3c1ae3ee33
hebasto:
ACK 3c1ae3ee33.
TheCharlatan:
ACK 3c1ae3ee33
Tree-SHA512: 1dd9d9933a5fceb9f8c4e1d68cd5cb4456a10a6dd27a6f6316f14493f9d2efad981ef8be9570c09ca82d45163aebd7f4cb2b2449989ec6084268ddba9a564c83
4357158c47 wallet: return and display signer error (Sjors Provoost)
dc55531087 wallet: compare address returned by displayaddress (Sjors Provoost)
6c1a2cc09a test: use h marker for external signer mock (Sjors Provoost)
Pull request description:
* HWI returns the requested address: as a sanity check, we now compare that to what we expected
* external signer documentation now reflects that HWI alternatives must implement this check
* both RPC and GUI will now return an error text, rather than just fail (the GUI even failed silently in some cases)
ACKs for top commit:
brunoerg:
ACK 4357158c47
achow101:
ACK 4357158c47
Tree-SHA512: 4f56edf3846745c8e7d08ef55cf29e8bb468256457149377c5f02da097931f9ca0c06bdbd856dc2385cde4fd11e4dc3b634c5a48814ff27f5562c8a25d43da93
55b13ecd2e doc: explain what the wallet password does (Brandon Odiwuor)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/18085
Add a document explaining what the wallet `passphrase` does
ACKs for top commit:
fjahr:
ACK 55b13ecd2e
achow101:
ACK 55b13ecd2e
Tree-SHA512: 6cc621269fe51ba7bd5d859427906a520e989bebcb274b08017c9900a4d781a165fc62dea46ed45aa48239cacd3c1f6eb4dd0c916be940d680a03266109716b0
21d0e6c7b7 doc: release notes for PR 27679 (Matthew Zipkin)
791dea204e test: cover unix sockets in zmq interface (Matthew Zipkin)
c87b0a0ff4 zmq: accept unix domain socket address for notifier (Matthew Zipkin)
Pull request description:
This is a follow-up to https://github.com/bitcoin/bitcoin/pull/27375, allowing ZMQ notifications to be published to a UNIX domain socket.
Fortunately, libzmq handles unix sockets already, all we really have to do to support it is allow the format in the actual option.
[libzmq](https://libzmq.readthedocs.io/en/latest/zmq_ipc.html) uses the prefix `ipc://` as opposed to `unix:` which is [used by Tor](https://gitlab.torproject.org/tpo/core/tor/-/blob/main/doc/man/tor.1.txt?ref_type=heads#L1475) and now also by [bitcoind](a85e5a7c9a/doc/release-notes-27375.md (L5)) so we need to switch that internally.
As far as I can tell, [LND](d20a764486/zmq.go (L38)) supports `ipc://` and `unix://` (notice the double slashes).
With this patch, LND can connect to bitcoind using unix sockets:
Example:
*bitcoin.conf*:
```
zmqpubrawblock=unix:/tmp/zmqsb
zmqpubrawtx=unix:/tmp/zmqst
```
*lnd.conf*:
```
bitcoind.zmqpubrawblock=ipc:///tmp/zmqsb
bitcoind.zmqpubrawtx=ipc:///tmp/zmqst
```
ACKs for top commit:
laanwj:
Code review ACK 21d0e6c7b7
tdb3:
crACK for 21d0e6c7b7. Changes lgtm. Will follow up with some testing within the next few days as time allows.
achow101:
ACK 21d0e6c7b7
guggero:
Tested and code review ACK 21d0e6c7b7
Tree-SHA512: ffd50222e80dd029d903e5ddde37b83f72dfec1856a3f7ce49da3b54a45de8daaf80eea1629a30f58559f4b8ded0b29809548c0638cd1c2811b2736ad8b73030
- Mention which directories contain the respective unsigned tarballs
- Clarify that bitcoin.conf might not need to be updated
- Specify where to put historical release notes if there is already
something in release-notes.md
- Clarify what exactly is the problem with running guix-codesign more
than once
- Correct number: 6 codesigned attestations are needed before uploading
binaries
- Remove scp command which is outdated
- Remove server path which is outdated
- Specify that translations update should happen before branch-off, not
before each release candidate
- Mention that you should notify lists when RCs are available
- Put "Archive the release notes" as a separate step, since creating the
github release has a dependency on it.
- Put bitcoincore.org website updates as a separate step, since
updating packaging repos may have a dependency on it.
- Update "bitcoin-dev mailing list" to "bitcoin-dev group"
- Document that maintainers should create PRs to collect backports
- Remove section about not uploading `*-debug` files, reader should
upload all build artifacts.
- Torrent is created automatically, so delete instructions.
- Mention that server also generates ots file automatically.
24b67fa9f6 doc: Add example of mixing private and public keys in descriptors (Anton A)
Pull request description:
closes: #27414
ACKs for top commit:
achow101:
ACK 24b67fa9f6
alfonsoromanz:
Re ACK 24b67fa9f6
Tree-SHA512: 8c063f23199ac0ff35909f786a5b0de1b4a9b15d1e93bdcdac10cb4bd2002c12e99b6fb1c2e56d16971e7622b67d910b79088429df92c48279be2d7797049911
78407b99ed [clang-tidy] Enable the misc-no-recursion check (dergoegge)
Pull request description:
Recursion is a frequent source of stack overflow bugs. Secondly, introduction of recursion can be non-obvious.
This PR proposes to use the clang-tidy `misc-no-recursion` check to make introduction of new recursion obvious. We don't make use of recursion a lot in our code base but there are a few places that need suppressions anyway (mostly the descriptor and univalue/rpc code).
ACKs for top commit:
stickies-v:
ACK 78407b99ed
TheCharlatan:
Re-ACK 78407b99ed
fanquake:
ACK 78407b99ed
Tree-SHA512: 34126d704c46086fe7371906ca852c25ced1dbd5fcfd85bf623810cd171a797569a92a33c7e26b8dc01c30c7bbf81aa326718926e8354585091411989a4edb14
The developer mailing list was migrated to Google Groups in February 2024
as announced in https://groups.google.com/g/bitcoindev/c/aewBuV6k-LI.
The archives maintained by the Linux Foundation stopped updating in December
2024. Thus, we point to the new archive maintained by gnusha.org.
The codebase refers to old discussions linked to the Linux Foundation archives.
Since all links are still active to this date, we keep them as they are.
See #29782.
80f8b92f4f remove libbitcoinconsensus (fanquake)
Pull request description:
This was deprecated in `v27.0`, for removal in `v28.0`. See discussion in PR #29189.
ACKs for top commit:
theuni:
Concept ACK and light review ACK 80f8b92f4f. My only hesitation here is that (afaics?) there's now nothing keeping undesired features like threading or globals from working their way into the interpreter in future commits.
m3dwards:
Concept ACK 80f8b92f4f
TheCharlatan:
ACK 80f8b92f4f
hebasto:
ACK 80f8b92f4f, I have reviewed the code and it looks OK.
Tree-SHA512: 17a62118aeb088f2695c892bb32794dfea3061e3cb7d9e8e9f1c06c3ff6f63a7587fa532e37edbb91fbc5a19b12c9a0f8e05fa9e8864aa07f92665375d847e80
430f319f73 depends: qt 5.15.13 (fanquake)
Pull request description:
I need to try and patch around Qts buildsystem to keep #21778 moving along (the issue being that even when you tell Qt to build using Clang on Linux, it still calls out to GCC, breaking our ability to have a macOS release build env that doesn't have a GCC toolchain installed, and thus no `ld` binary).
Before trying to patch Qt any further, update to the latest LTS release, and update the current patch set.
ACKs for top commit:
TheCharlatan:
Re-ACK 430f319f73
hebasto:
re-ACK 430f319f73.
Tree-SHA512: 11122c04d63f4ec79e5d7945e1686eaf308cb1f98c3ff8bc52e265d7353cd4f1a532f4d78d55b43183245aeebf93cc1092087d9fe83e42eefa8a69ad9017d2da
Pretty much all library packages were renamed in the 64-bit time_t
migration to add `t64` (even on 64-bit platforms).
Instead of complicating the doc with conditional package names, suggest
installing the `-dev` packages which still have the same name, and
besides that, are the right way to go about it as they contain the
"user facing" C++ headers needed to build against Qt5.
For Fedora, devel packages are already suggested.
This affects Ubuntu 24.04 and Debian Testing.
I need to try and patch around Qts buildsystem to keep #21778 moving
along (the issue being that even when you tell Qt to build using
Clang on Linux, it still calls out to GCC, breaking our ability to have
a macOS release build env that doesn't have a GCC toolchain installed,
and thus no ld binary).
Before trying to patch Qt any further, update to the latest LTS
release, and update the current patch set.
567cec9a05 doc: add release notes and help text for unix sockets (Matthew Zipkin)
bfe5192891 test: cover UNIX sockets in feature_proxy.py (Matthew Zipkin)
c65c0d0163 init: allow UNIX socket path for -proxy and -onion (Matthew Zipkin)
c3bd43142e gui: accomodate unix socket Proxy in updateDefaultProxyNets() (Matthew Zipkin)
a88bf9dedd i2p: construct Session with Proxy instead of CService (Matthew Zipkin)
d9318a37ec net: split ConnectToSocket() from ConnectDirectly() for unix sockets (Matthew Zipkin)
ac2ecf3182 proxy: rename randomize_credentials to m_randomize_credentials (Matthew Zipkin)
a89c3f59dc netbase: extend Proxy class to wrap UNIX socket as well as TCP (Matthew Zipkin)
3a7d6548ef net: move CreateSock() calls from ConnectNode() to netbase methods (Matthew Zipkin)
74f568cb6f netbase: allow CreateSock() to create UNIX sockets if supported (Matthew Zipkin)
bae86c8d31 netbase: refactor CreateSock() to accept sa_family_t (Matthew Zipkin)
adb3a3e51d configure: test for unix domain sockets (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/27252
UNIX domain sockets are a mechanism for inter-process communication that are faster than local TCP ports (because there is no need for TCP overhead) and potentially more secure because access is managed by the filesystem instead of serving an open port on the system.
There has been work on [unix domain sockets before](https://github.com/bitcoin/bitcoin/pull/9979) but for now I just wanted to start on this single use-case which is enabling unix sockets from the client side, specifically connecting to a local Tor proxy (Tor can listen on unix sockets and even enforces strict curent-user-only access permission before binding) configured by `-onion=` or `-proxy=`
I copied the prefix `unix:` usage from Tor. With this patch built locally you can test with your own filesystem path (example):
`tor --SocksPort unix:/Users/matthewzipkin/torsocket/x`
`bitcoind -proxy=unix:/Users/matthewzipkin/torsocket/x`
Prep work for this feature includes:
- Moving where and how we create `sockaddr` and `Sock` to accommodate `AF_UNIX` without disturbing `CService`
- Expanding `Proxy` class to represent either a `CService` or a UNIX socket (by its file path)
Future work:
- Enable UNIX sockets for ZMQ (https://github.com/bitcoin/bitcoin/pull/27679)
- Enable UNIX sockets for I2P SAM proxy (some code is included in this PR but not tested or exposed to user options yet)
- Enable UNIX sockets on windows where supported
- Update Network Proxies dialog in GUI to support UNIX sockets
ACKs for top commit:
Sjors:
re-ACK 567cec9a05
tdb3:
re ACK for 567cec9a05.
achow101:
ACK 567cec9a05
vasild:
ACK 567cec9a05
Tree-SHA512: de81860e56d5de83217a18df4c35297732b4ad491e293a0153d2d02a0bde1d022700a1131279b187ef219651487537354b9d06d10fde56225500c7e257df92c1
0a533613fb docs: add release notes for #27114 (brunoerg)
e6b8f19de9 test: add coverage for whitelisting manual connections (brunoerg)
c985eb854c test: add option to speed up tx relay/mempool sync (brunoerg)
66bc6e2d17 Accept "in" and "out" flags to -whitelist to allow whitelisting manual connections (Luke Dashjr)
8e06be347c net_processing: Move extra service flag into InitializeNode (Luke Dashjr)
9133fd69a5 net: Move `NetPermissionFlags::Implicit` verification to `AddWhitelistPermissionFlags` (Luke Dashjr)
2863d7dddb net: store `-whitelist{force}relay` values in `CConnman` (brunoerg)
Pull request description:
Revives #17167. It allows whitelisting manual connections. Fixes#9923
Since there are some PRs/issues around this topic, I'll list some motivations/comments for whitelisting outbound connections from them:
- Speed-up tx relay/mempool sync for testing purposes (my personal motivation for this) - In #26970, theStack pointed out that we whitelist peers to speed up tx relay for fast mempool synchronization, however, since it applies only for inbound connections and considering the topology `node0 <--- node1 <---- node2 <--- ... <-- nodeN`, if a tx is submitted from any node other than node0, the mempool synchronization can take quite long.
- https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1865155764 - "Before enabling -v2transport by default (which I'd image may happen after https://github.com/bitcoin/bitcoin/pull/24748) we could consider a way to force manual connections to be only-v1 or even only-v2 (disabling reconnect-with-v1). A possibility could be through a net permission flag, if https://github.com/bitcoin/bitcoin/pull/27114 makes it in."
- https://github.com/bitcoin/bitcoin/pull/17167#issuecomment-1168606032 - "This would allow us to use https://github.com/bitcoin/bitcoin/pull/25355 when making outgoing connections to all nodes, except to whitelisted ones for which we would use our persistent I2P address."
- Force-relay/mempool permissions for a node you intentionally connected to.
ACKs for top commit:
achow101:
ACK 0a533613fb
sr-gi:
re-ACK [0a53361](0a533613fb)
pinheadmz:
ACK 0a533613fb
Tree-SHA512: 97a79bb854110da04540897d2619eda409d829016aafdf1825ab5515334b0b42ef82f33cd41587af235b3af6ddcec3f2905ca038b5ab22e4c8a03d34f27aebe1
