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.
It is best to store all key origin information
(master key fingerprint and all derivation steps)
in the multisig descriptor. Being explicit with
this information should be beneficial if this approach
is used with other wallets/signers (whether hardware
or software). There is no harm including all of this
with xpubs (if anything it simplifies the test code)
and makes this example/docs more complete and safer
incase it is referenced by others.
cbc6c440e3811d342fa570713702900b3e3e75b9 doc: add comments and release-notes for JSON-RPC 2.0 (Matthew Zipkin)
e7ee80dcf2b68684eae96070875ea13a60e3e7b0 rpc: JSON-RPC 2.0 should not respond to "notifications" (Matthew Zipkin)
bf1a1f1662427fbf1a43bb951364eface469bdb7 rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests (Matthew Zipkin)
466b90562f4785de74b548f7c4a256069e2aaf43 rpc: Add "jsonrpc" field and drop null "result"/"error" fields (Matthew Zipkin)
2ca1460ae3a7217eaa8c5972515bf622bedadfce rpc: identify JSON-RPC 2.0 requests (Matthew Zipkin)
a64a2b77e09bff784a2635ba19ff4aa6582bb5a5 rpc: refactor single/batch requests (Matthew Zipkin)
df6e3756d6feaf1856e7886820b70874209fd90b rpc: Avoid copies in JSONRPCReplyObj() (Matthew Zipkin)
09416f9ec445e4d6bb277400758083b0b4e8b174 test: cover JSONRPC 2.0 requests, batches, and notifications (Matthew Zipkin)
4202c170da37a3203e05a9f39f303d7df19b6d81 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 cbc6c440e3811d342fa570713702900b3e3e75b9
ryanofsky:
Code review ACK cbc6c440e3811d342fa570713702900b3e3e75b9. 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 cbc6c440e3811d342fa570713702900b3e3e75b9
Tree-SHA512: 0b702ed32368b34b29ad570d090951a7aeb56e3b0f2baf745bd32fdc58ef68fee6b0b8fad901f1ca42573ed714b150303829cddad4a34ca7ad847350feeedb36
42fb5311b19582361409d65c6fddeadbee14bb97 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 401926283a200994ecd7df8eae8ced8e0b067c46, 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 ef2a3de25c882396e1776b554878d2784b6b7391, 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 42fb5311b19582361409d65c6fddeadbee14bb97
tdb3:
Re ACK for 42fb5311b19582361409d65c6fddeadbee14bb97
TheCharlatan:
ACK 42fb5311b19582361409d65c6fddeadbee14bb97
maflcko:
ACK 42fb5311b19582361409d65c6fddeadbee14bb97 🔺
Tree-SHA512: 4225ed8979cd5f030dec785a80e7452a041ad5703445da79d2906ada983ed0bbe7b15889d663d75aae4a77d92e302c93e93eca185c7bd47c9cce29e12f752bd3
Previously, our Android builds were geared towards generating APKs,
which relied on Qt. However, after migrating to C++20, compiling for
Android became unfeasible due to Qt 5.15's compatibility limitations
with NDK only up to r25, which includes an outdated embedded libc++.
All removed stuff will be reinstated after migrating the build system to
CMake and upgrading Qt to version 6.x."
22574046c90c0662f3aa9b1baea074aff54f92a9 doc: add LLVM instruction for macOS < 13 (Sjors Provoost)
Pull request description:
#29208 bumped clang to 14, which users of old macOS versions need to install manually. This PR adds instructions.
Xcode 14.3.1 ships clang 14.0.3 (14.0.0 is broken, see #29918):
https://en.wikipedia.org/wiki/Xcode#Xcode_11.0_-_14.x_(since_SwiftUI_framework)
The system requirements for that is macOS Ventura 13.0 or later: https://developer.apple.com/documentation/xcode-release-notes/xcode-14_3_1-release-notes#
Homebrew itself officially supports macOS 12 or later, but _may_ still work on macOS 11: https://docs.brew.sh/Installation
Fwiw macOS 11 Big Sur last got an update in September 2023, so Apple has not _entirely_ written it off: https://en.wikipedia.org/wiki/MacOS_Big_Sur
ACKs for top commit:
maflcko:
utACK 22574046c90c0662f3aa9b1baea074aff54f92a9
TheCharlatan:
ACK 22574046c90c0662f3aa9b1baea074aff54f92a9
Tree-SHA512: 5b4bcc71966d1da84bc4da32da89e0dea9f519f37d9e14e169140c92af044b33f404f01ae7d10f53ab5345dd51ac404c161389efef93da5cacbfd52a43881695
5195baa60087ee366290887ad982fc491e14c111 depends: fix miniupnpc snprintf usage on Windows (fanquake)
3c2d440f1497f60bb444326f51383df244dcdfe9 depends: switch miniupnpc to CMake (Cory Fields)
f5618c79d9e7af05c2987044dc2da03697f8bb89 depends: add upstream CMake patch to miniupnpc (fanquake)
6866b571ab96f03ca0775424e45458c5731f230f 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 5195baa60087ee366290887ad982fc491e14c111.
TheCharlatan:
utACK 5195baa60087ee366290887ad982fc491e14c111
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.
1ea8674316f2dce0005f6094b6ee111b045dd770 [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 1ea8674316f2dce0005f6094b6ee111b045dd770
Tree-SHA512: 4ad10d4ce2c33fe15cb02599353107bb72ecb867aefc6c120cfd5cdea42aa8fa3783f9e0218c2f3815f030e0694cc8fb24011ce88358a0206cb07416a256a962
2179e2c3209a41c1419f1f5ed6270a0dad68b50d 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 2179e2c3209a41c1419f1f5ed6270a0dad68b50d
davidgumberg:
ACK 2179e2c320
achow101:
ACK 2179e2c3209a41c1419f1f5ed6270a0dad68b50d
byaye:
ACK 2179e2c3209a41c1419f1f5ed6270a0dad68b50d
Tree-SHA512: 18a6a627343fb0aa824029d99df8a232153ba288ce94ec8c5da25693885237381fba505ea1e71c756b2a611243a302d319ca7ae03b526020cd6588710fc2ac17
3c1ae3ee33d4d9dbea046d5ab8ee924a12982759 depends: switch libnatpmp to CMake (Cory Fields)
72ba7b5d263b6d909ae59040536a499a596237c2 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 3c1ae3ee33d4d9dbea046d5ab8ee924a12982759.
TheCharlatan:
ACK 3c1ae3ee33d4d9dbea046d5ab8ee924a12982759
Tree-SHA512: 1dd9d9933a5fceb9f8c4e1d68cd5cb4456a10a6dd27a6f6316f14493f9d2efad981ef8be9570c09ca82d45163aebd7f4cb2b2449989ec6084268ddba9a564c83
4357158c4712d479522d5cd441ad4dd1693fdd05 wallet: return and display signer error (Sjors Provoost)
dc55531087478d01fbde4f5fbb75375b672960c3 wallet: compare address returned by displayaddress (Sjors Provoost)
6c1a2cc09a00baa6ff3ff34455c2243b43067fb5 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 4357158c4712d479522d5cd441ad4dd1693fdd05
achow101:
ACK 4357158c4712d479522d5cd441ad4dd1693fdd05
Tree-SHA512: 4f56edf3846745c8e7d08ef55cf29e8bb468256457149377c5f02da097931f9ca0c06bdbd856dc2385cde4fd11e4dc3b634c5a48814ff27f5562c8a25d43da93
55b13ecd2e00ad2dbfd44c34d7de6f616590adf8 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 55b13ecd2e00ad2dbfd44c34d7de6f616590adf8
achow101:
ACK 55b13ecd2e00ad2dbfd44c34d7de6f616590adf8
Tree-SHA512: 6cc621269fe51ba7bd5d859427906a520e989bebcb274b08017c9900a4d781a165fc62dea46ed45aa48239cacd3c1f6eb4dd0c916be940d680a03266109716b0
21d0e6c7b7c7af7f6e54a45829b4fbfba6923b86 doc: release notes for PR 27679 (Matthew Zipkin)
791dea204ecde9b500ec243b4e16fc601998ec84 test: cover unix sockets in zmq interface (Matthew Zipkin)
c87b0a0ff4cb6d83bb59360ac4453f6daa871177 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 21d0e6c7b7c7af7f6e54a45829b4fbfba6923b86
tdb3:
crACK for 21d0e6c7b7c7af7f6e54a45829b4fbfba6923b86. Changes lgtm. Will follow up with some testing within the next few days as time allows.
achow101:
ACK 21d0e6c7b7c7af7f6e54a45829b4fbfba6923b86
guggero:
Tested and code review ACK 21d0e6c7b7c7
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.
24b67fa9f602cdeac0e9736256f77d048f616c48 doc: Add example of mixing private and public keys in descriptors (Anton A)
Pull request description:
closes: #27414
ACKs for top commit:
achow101:
ACK 24b67fa9f602cdeac0e9736256f77d048f616c48
alfonsoromanz:
Re ACK 24b67fa9f602cdeac0e9736256f77d048f616c48
Tree-SHA512: 8c063f23199ac0ff35909f786a5b0de1b4a9b15d1e93bdcdac10cb4bd2002c12e99b6fb1c2e56d16971e7622b67d910b79088429df92c48279be2d7797049911
78407b99ed6dd17f687fcbfb0486ecc433302287 [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 78407b99ed6dd17f687fcbfb0486ecc433302287
TheCharlatan:
Re-ACK 78407b99ed6dd17f687fcbfb0486ecc433302287
fanquake:
ACK 78407b99ed6dd17f687fcbfb0486ecc433302287
Tree-SHA512: 34126d704c46086fe7371906ca852c25ced1dbd5fcfd85bf623810cd171a797569a92a33c7e26b8dc01c30c7bbf81aa326718926e8354585091411989a4edb14
0ead466a0c72bef0a8622749b84e9c7c5c37144f Update the developer mailing list address. (Edil Medeiros)
Pull request description:
The developer mailing list was migrated to Google Groups in February 2024 as announced in https://groups.google.com/g/bitcoindev/c/aewBuV6k-LI.
In this patch, I decided to add a link to the [archives maintained by the Linux Foundation](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/) as linked in the [old mailing list page](https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev).
A reasonable alternative would be link to the [new archives](https://gnusha.org/pi/bitcoindev/) linked in the [migration announcement message](https://groups.google.com/g/bitcoindev/c/aewBuV6k-LI). I'm not sure about the status of the archive migration, probably the old archives are more comprehensive to this date.
ACKs for top commit:
josibake:
reACK 0ead466a0c
Sjors:
ACK 0ead466a0c72bef0a8622749b84e9c7c5c37144f
Zero-1729:
crACK 0ead466a0c72bef0a8622749b84e9c7c5c37144f
Tree-SHA512: c29296a45a65f342df03faffa49e7dc1469d78e96074a7dbc82d89fc3f45179e4828015cbb6400b44830d6bb7fc23869abf2b8a7070196b03f99fbb0158bb343
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.
80f8b92f4f2311b9e9a25361c9dd973244e6f95c 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 80f8b92f4f2311b9e9a25361c9dd973244e6f95c. 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 80f8b92f4f2311b9e9a25361c9dd973244e6f95c
hebasto:
ACK 80f8b92f4f2311b9e9a25361c9dd973244e6f95c, I have reviewed the code and it looks OK.
Tree-SHA512: 17a62118aeb088f2695c892bb32794dfea3061e3cb7d9e8e9f1c06c3ff6f63a7587fa532e37edbb91fbc5a19b12c9a0f8e05fa9e8864aa07f92665375d847e80
430f319f73894499c56021b70f2827475c3f0433 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 430f319f73894499c56021b70f2827475c3f0433
hebasto:
re-ACK 430f319f73894499c56021b70f2827475c3f0433.
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.
