Age | Commit message (Collapse) | Author |
|
|
|
|
|
Implement ReadKey and HasKey of BerkeleyROBatch, and Next of BerkeleyROCursor.
Also adds the containers for records to BerkeleyRODatabase so that
BerkeleyROBatch will be able to access the records.
|
|
BerkeleyRODatabase and BerkeleyROBatch will be used to access a BDB file
without the use of BDB. For now, these are dummy classes.
|
|
Rather than using std::ostringstream and manually joining the
comments, use strprintf and our own `Join` helper.
|
|
flush: >2x block connection speed
4a6d1d1e3bc3d559cd13d134b4b827f22635ac4b validation: don't clear cache on periodic flush (Andrew Toth)
Pull request description:
Since https://github.com/bitcoin/bitcoin/pull/17487 we no longer need to clear the coins cache when syncing to disk. A warm coins cache significantly speeds up block connection, and only needs to be fully flushed when nearing the `dbcache` limit.
Periodic flushes occur every 24 hours, which empties the cache and causes block connection to slow down. By keeping the cache through periodic flushes a node can run for several days with an increasingly hotter cache and connect blocks much more quickly. Now not only can setting a higher `dbcache` value be beneficial for IBD, it can also be beneficial for connecting blocks faster.
To benchmark in real world usage, I spun up 6 identical `t2.small` AWS EC2 instances, all running in the same region in the same VPC. I configured 2 instances to run master, 2 instances to run the change in this PR, and 2 instances to run the change in this PR but with `dbcache=1000`. All instances had `prune=5000` and a 20 GB `gp2` `EBS` volume. A 7th EC2 instance in the same VPC ran master and connected only to some trusted nodes in the outside network. Each of the 6 nodes under test only connected directly to this 7th instance. I manually pruned as much as possible and uploaded the same `blocks`, `chainstate` and `mempool.dat` to all instances. I started all 6 peers simultaneously at block height `835245` and ran them for over a week until block `836534`.
The results were much faster block connection times for this branch compared to master, and much faster for this branch with `dbcache=1000` compared to default `dbcache`.
| branch |speed |
|-----------:|----------:|
| master 1 | 1995.49ms/blk |
| master 2 | 2129.78ms/blk |
| branch default dbcache 1 | 1189.65ms/blk |
| branch default dbcache 2 | 1037.74ms/blk |
| branch dbcache=1000 1 | 393.69ms/blk |
| branch dbcache=1000 2 | 427.77ms/blk |
The log files of all 6 instances are [here](https://gist.github.com/andrewtoth/03c95033e7581d5dbc5be028639a1a91).
There is a lot of noise with the exact times of blocks being connected, so I plotted the rolling 20 block connect time averages. The large dots are the times where the cache is emptied. For the red master nodes, this happens every 24 hours. The blue branch nodes with default `dbcache` only filled up and emptied the caches once, which is seen in the middle. The green branch nodes with 1000 `dbcache` never emptied the cache. It is very clear from the chart that whenever the cache is emptied, connect block speed degrades significantly.

Also note that this still clears the cache for pruning flushes. Having frequent pruning flushes with a large cache that doesn't clear is less performant than the status quo https://github.com/bitcoin/bitcoin/pull/15265#issuecomment-458657451. See https://github.com/bitcoin/bitcoin/pull/28280.
ACKs for top commit:
sipa:
utACK 4a6d1d1e3bc3d559cd13d134b4b827f22635ac4b
achow101:
ACK 4a6d1d1e3bc3d559cd13d134b4b827f22635ac4b
brunoerg:
crACK 4a6d1d1e3bc3d559cd13d134b4b827f22635ac4b
Tree-SHA512: 05dbc677bc309bbcf89c52a6c5e853e2816b0ef0b5ee3719b30696df315a0427e244bb82da9ad828ec0e7ea8764552f8affe14c0184b52adf1909f5d8c1b4f9e
|
|
b77bad309e92f176f340598eec056eb7bff86f5f rpc: move UniValue in blockToJSON (willcl-ark)
Pull request description:
Fixes: #24542
Fixes: #30052
Without explicitly declaring the move, these `UniValues` get copied, causing increased memory usage. Fix this by explicitly moving the `UniValue` objects.
Used by `rest_block` and `getblock` RPC.
ACKs for top commit:
maflcko:
review ACK b77bad309e92f176f340598eec056eb7bff86f5f
ismaelsadeeq:
ACK b77bad309e92f176f340598eec056eb7bff86f5f
TheCharlatan:
ACK b77bad309e92f176f340598eec056eb7bff86f5f
theuni:
utACK b77bad309e92f176f340598eec056eb7bff86f5f
hebasto:
ACK b77bad309e92f176f340598eec056eb7bff86f5f, I have reviewed the code and it looks OK.
BrandonOdiwuor:
ACK b77bad309e92f176f340598eec056eb7bff86f5f
Tree-SHA512: 767608331040f9cfe5c3568ed0e3c338920633472a1a50d4bbb47d1dc69d2bb11466d611f050ac8ad1a894b47fe1ea4d968cf34cbd44d4bb8d479fc5c7475f6d
|
|
Adds a warning when both -rpcconnect and -rpcport define
a port to be used.
|
|
Adds error handling of invalid ports to rpcconnect and rpcport,
with associated functional tests.
|
|
58594c7040241f2312b0b8735a8baf0412674613 fuzz: txorphan tests fixups (Sergi Delgado Segura)
Pull request description:
Motivated by https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1576401327
Adds the following fixups in txorphan fuzz tests:
- Don't bond the output count of the created orphans to the number of available coins
- Allow duplicate inputs but don't store duplicate outpoints
Most significantly, this gets rid of the `duplicate_input` flag altogether, making the test easier to reason about. Notice how, under normal conditions, duplicate inputs would be caught by `MemPoolAccept::PreChecks`, however, no validations checks are run on the test before adding data to the orphanage (neither were they before this patch)
## Rationale
The way the test is currently written, duplicate inputs are allowed based on a random flag (`duplicate_input`). If the flag is unset, upon selecting an outpoint as input for a new transaction, the input is popped to prevent re-selection and later re-added to the collection (once all inputs have been picked). However, the re-addition to the collection is performed independently of whether the flag was set or not. This means that, if the flag is set, the selected inputs are duplicated which in turn makes these inputs more likely to be re-picked in the following iteration of the loop.
Additionally, both the input and output count of the transaction are bonded to the number of available outpoints. This makes sense for the former, but the latter shouldn't be.
ACKs for top commit:
maflcko:
utACK 58594c7040241f2312b0b8735a8baf0412674613
glozow:
ACK 58594c7
instagibbs:
ACK 58594c7040241f2312b0b8735a8baf0412674613
Tree-SHA512: e97cc2a43e388f87b64d2e4e45f929dd5b0dd85d668dd693b75e4c9ceea734cd7645952385d428208d07b70e1aafbec84cc2ec264a2e07d36fc8ba3e97885a8d
|
|
not connect automatically
95897ff181c0757e445f0e066a2a590a0a0120d2 doc: removed help text saying that peers may not connect automatically (kevkevin)
Pull request description:
Introduced in https://github.com/bitcoin/bitcoin/pull/23542 and released in version 23.0 there has been significant time since this change (2 years).
This should be removed as it is no longer relevant
ACKs for top commit:
stickies-v:
ACK 95897ff181c0757e445f0e066a2a590a0a0120d2
tdb3:
ACK for 95897ff181c0757e445f0e066a2a590a0a0120d2.
vasild:
ACK 95897ff181c0757e445f0e066a2a590a0a0120d2
jonatack:
ACK 95897ff181c0757e445f0e066a2a590a0a0120d2
kristapsk:
ACK 95897ff181c0757e445f0e066a2a590a0a0120d2. According to https://bitnodes.io/dashboard/#user-agents stats, most nodes on the network are v23+.
Tree-SHA512: 9e35194f8a1e06f1447450af2ea30cdc43722665c2d2e4b7aa9a52afac5c1e83fed744742c836743a555cc180c90f9eebdc6637eba6190010d693eef9c5834f7
|
|
Without explicitly declaring the move, these UniValues get copied,
causing increased memory usage. Fix this by explicitly moving the
UniValue objects.
Used by `rest_block` and `getblock` RPC.
|
|
Bech32(m) was defined with a 90 character limit so that certain
guarantees for error detection could be made for segwit addresses.
However, there is nothing about the encoding scheme itself that requires
a limit and in practice bech32(m) has been used without the 90 char
limit (e.g. lightning invoices).
Further, increasing the character limit doesn't do away with error
detection, it simply lessons the guarantees.
Model charlimit as an Enum, so that if a different address scheme is
using bech32(m), the character limit for that address scheme can be
used, rather than always using the 90 charlimit defined for segwit
addresses.
upate comment
|
|
`ProduceSignature` already calls `VerifyScript` internally as last step in
order to check whether the signature data is complete. If and only if that is
the case, the `complete` field of the `SignatureData` is set accordingly and
there is no need then to verify the script after again, as we already know that
it would succeed.
This leads to a rough ~20% speed-up for `SignTransaction` for single-input
ECDSA or Taproot inputs, according to the `SignTransaction{ECDSA,Taproot}`
benchmarks.
|
|
|
|
671b7a32516d62e1e79393ded4b45910bd08010a gui: fix create unsigned transaction fee bump (furszy)
Pull request description:
Fixes #810.
Not much to explain; we were requiring the wallet to be unlocked for the unsigned transaction creation process.
Fix this by moving the unlock wallet request to the signed transaction creation process.
ACKs for top commit:
pablomartin4btc:
tACK 671b7a32516d62e1e79393ded4b45910bd08010a
hebasto:
ACK 671b7a32516d62e1e79393ded4b45910bd08010a, tested on Ubuntu 24.04.
Tree-SHA512: 5b9ec5a1b91c014c05c83c63daaa8ba33f9dc1bfa930442315a0913db710df17a1b6bb4ad39f1419a7054f37ebedb7ad52e1c5d3d2fb444b1676162e89a4efd2
|
|
trailing newline, so don't add an extra one
d1ed09a7643b567e021b2ecb756bc925c48fc708 Bugfix: GUI: Help messages already have a trailing newline, so don't add an extra one (Luke Dashjr)
Pull request description:
Reviewing #29585, I noticed that `bitcoin-qt` adds an extra newline for `-help` and `-version` beyond the other binaries'.
ACKs for top commit:
hebasto:
ACK d1ed09a7643b567e021b2ecb756bc925c48fc708, tested on Ubuntu 24.04.
Tree-SHA512: 15ee9d1060c2492bb3b04a0ac4cb53f7b959bbe32bce415793da0c221f1c963c8f2bb3996ea07d1a7c192bfc2e23be2cd7d4e5649c592eb3fc03906c2763f1aa
|
|
10c5275ba4532fb1bf54057d2f61fc35b51f1e85 gui: don't permit port in proxy IP option (willcl-ark)
Pull request description:
Fixes: https://github.com/bitcoin-core/gui/issues/809
Previously it was possible through the GUI to enter an IP address:port into the "Proxy IP" configuration box. After the node was restarted the errant setting would prevent the node starting back up until manually removed from settings.json.
Prevent this with a simple check for ":" in the string. This is acceptable here in the GUI setting because we already fail on a hostname such as "http://x.x.x.x", so it won't cause false positives.
ACKs for top commit:
furszy:
utACK 10c5275ba45
hebasto:
ACK 10c5275ba4532fb1bf54057d2f61fc35b51f1e85, tested on Ubuntu 24.04.
Tree-SHA512: ed83590630cf693680a3221f701ecd18dd08710a17b726dc4978a3a6e330a34fb77d73a4f710c01bcb3faf88b6604ff37bcdbb191ce1623348ca5b92fd6fe9a7
|
|
3bf00e13609eefa6ddb11353519bb1aec2342513 gui: debugwindow: update session ID tooltip (Marnix)
Pull request description:
When you have a v2 connection, there is always a session ID.
the _if any_ is a leftover from https://github.com/bitcoin-core/gui/pull/754, where the session ID property initially would always be displayed (transport v1 and v2).
So the session ID could be empty when you have a v1 connection.
As now the _Session ID_ property only is displayed for v2 connection, there will always be a session ID.
master

PR

Session ID not shown when transport v1

<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
vostrnad:
ACK 3bf00e13609eefa6ddb11353519bb1aec2342513
kristapsk:
ACK 3bf00e13609eefa6ddb11353519bb1aec2342513
jarolrod:
ACK 3bf00e13609eefa6ddb11353519bb1aec2342513
pablomartin4btc:
tACK 3bf00e13609eefa6ddb11353519bb1aec2342513
hebasto:
ACK 3bf00e13609eefa6ddb11353519bb1aec2342513.
Tree-SHA512: 4de0c56c070dc5d1cee73b629bdf3d1778c6d90d512337aa6cfd3eed4ce95cbcfbe5713e2942f6fc22907b2c4d9df7979ba8e9f91f7cc173b42699ea35113f96
|
|
`subprocess.h` header
5a11d3023f7d0cde777f3496c0f3aa381823d749 refactor, subprocess: Remove unused stream API calls (Hennadii Stepanov)
05b6f8793c6d5f17d1cb413e2884f1fb0f367ad8 refactor, subprocess: Remove unused `Popen::child_created_` data member (Hennadii Stepanov)
9e1ccf55e178144804e30cfe94757b6da2a6ca28 refactor, subprocess: Remove unused `Popen::poll()` (Hennadii Stepanov)
24b53fc84af301fff592e06723b980e29aa68289 refactor, subprocess: Remove `Popen::pid()` (Hennadii Stepanov)
Pull request description:
This PR continues https://github.com/bitcoin/bitcoin/pull/29961.
Please note that `Popen::poll()` is not required for https://github.com/bitcoin/bitcoin/pull/29868 anymore.
ACKs for top commit:
theuni:
Easy code review ACK 5a11d3023f7d0cde777f3496c0f3aa381823d749 since it's all removals :)
theStack:
Code-review ACK 5a11d3023f7d0cde777f3496c0f3aa381823d749
Tree-SHA512: 11f984f8384c337782d058afa80011e88087a1b5a3ed6e4678d492e6c232b706a26312463c5dd8c529aa477497c8afca9f54574e0e36e3edd5675bd5d8424bbb
|
|
|
|
Addnode (manual) peers connected to us via the cjdns network are currently not
detected by CConnman::GetAddedNodeInfo(), i.e. fConnected is always false.
This causes the following issues:
- RPC `getaddednodeinfo` incorrectly shows them as not connected
- CConnman::ThreadOpenAddedConnections() continually retries to connect them
|
|
96378fe734e5fb6167eb20036d7170572a647edb Refactor: Remove ECC_Start and ECC_Stop from key header (TheCharlatan)
41eba5bd716bea47c8731d156d053afee92a7f12 kernel: Remove key module from kernel library (TheCharlatan)
a08d2b3cb971c68e9a50b991b2953fa4541cf48a tools: Use ECC_Context helper in bitcoin-tx and bitcoin-wallet tools (Ryan Ofsky)
28905c1a64a87a56f16aea8a4d23dea7eec9ca59 test: Use ECC_Context helper in bench and fuzz tests (Ryan Ofsky)
538fedde1d9c96a2bbe06cacc0cd6903135fbc83 common: Add ECC_Context RAII wrapper for ECC_Start/ECC_Stop (Ryan Ofsky)
Pull request description:
The key module's functionality is not used by the kernel library, but currently kernel users are still required to initialize the key module's `secp256k1_context_sign` global as part of the `kernel::Context` through `ECC_Start`. So move the `ECC_Start` call to the `NodeContext` ctor instead to completely remove the key module from the kernel library.
The gui tests currently keep multiple `NodeContext` objects in memory, so call `ECC_Stop` manually to avoid triggering an assertion on `ECC_Start`.
---
This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587). It removes a module from the kernel library.
ACKs for top commit:
achow101:
ACK 96378fe734e5fb6167eb20036d7170572a647edb
ryanofsky:
Code review ACK 96378fe734e5fb6167eb20036d7170572a647edb. Just suggested comment changes since last review.
theuni:
utACK 96378fe734e5fb6167eb20036d7170572a647edb
Tree-SHA512: 40be427e8e2c920c0e3ce64a9bdd90551be27a89af11440bfb6ab0dd3a1d1ccb7cf1f82383cd782818cd1bb44d5ae5d2161cf4d78d3127ce4987342007090bab
|
|
|
|
|
|
|
|
|
|
updates comment in ConsiderEviction
d53d84834747c37f4060a9ef379e0a6b50155246 test: adds outbound eviction tests for non outbound-full-relay peers (Sergi Delgado Segura)
a8d9a0edc7cef2c31a557ef53eb45520976b0d65 test: adds outbound eviction functional tests, updates comment in ConsiderEviction (Sergi Delgado Segura)
Pull request description:
## Motivation
While checking the outbound eviction code I realized a case was not considered within the comments, which in turn made me realize we had no functional tests for the outbound eviction case (when I went to check/add the test case).
This PR updates the aforementioned comment and adds functional tests to cover the outbound eviction logic, in addition to the existing unit tests found at `src/test/denialofservice_tests.cpp`.
ACKs for top commit:
davidgumberg:
reACK https://github.com/bitcoin/bitcoin/commit/d53d84834747c37f4060a9ef379e0a6b50155246
tdb3:
Re ACK for d53d84834747c37f4060a9ef379e0a6b50155246
achow101:
ACK d53d84834747c37f4060a9ef379e0a6b50155246
cbergqvist:
ACK d53d84834747c37f4060a9ef379e0a6b50155246
Tree-SHA512: 633b84bb1229fe21e2f650c1beada33ca7f190b64eafd64df2266516d21175e5d652e019ff7114f00cb8bd19f5817dc19e65adf75767a88e24dc0842ce40c63e
|
|
They are unused outside of the key module now.
|
|
The key module's functionality is not used by the kernel library, but
currently kernel users are still required to initialize the key module's
`secp256k1_context_sign` global as part of the `kernel::Context` through
`ECC_Start`.
|
|
|
|
|
|
|
|
other improvements
78e52f663f3e3ac86260b913dad777fd7218f210 doc: rpc: fix submitpackage examples (stickies-v)
1a875d4049574730d4a53a1b68bd29b80ad96d38 rpc: update min package size error message in submitpackage (stickies-v)
f9ece258aa868d0776caa86b94e71ba05a9b287e doc: rpc: submitpackage takes sorted array (stickies-v)
17f74512f0d19cb452ed79a4bff5a222fcdb53c4 test: add bounds checking for submitpackage RPC (stickies-v)
Pull request description:
`submitpackage` requires the package to be topologically sorted with the child being the last element in the array, but this is not documented in the RPC method or the error messages.
Also sneaking in some other minor improvements that I found while going through the code:
- Informing the user that `package` needs to be an array of length between `1` and `MAX_PACKAGE_COUNT` is confusing when `IsChildWithPackage()` requires that the package size >= 2. Remove this check to avoid code duplication and sending a confusing error message.
- fixups to the `submitpackage` examples
ACKs for top commit:
fjahr:
re-ACK 78e52f663f3e3ac86260b913dad777fd7218f210
instagibbs:
ACK https://github.com/bitcoin/bitcoin/pull/29292/commits/78e52f663f3e3ac86260b913dad777fd7218f210
achow101:
ACK 78e52f663f3e3ac86260b913dad777fd7218f210
glozow:
utACK 78e52f663f3e3ac86260b913dad777fd7218f210
Tree-SHA512: a8845621bb1cbf784167fc7c82cb8ceb105868b65b26d3465f072d1c04ef3699e85a21a524ade805d423bcecbc34f7d5bff12f2c21cbd902ae1fb154193ebdc9
|
|
fKnown is true during reindex (and only then), which deletes
any existing snapshot chainstate. As a result, this function can never
be called wth fKnown set and a snapshot chainstate.
Add an Assume for this, and make the code initializing a blockfile cursor
for the snapshot conditional on !fKnown.
This is a preparation for splitting the reindex logic out of
FindBlockPos in the following commits.
|
|
net processing
75d27fefc7a04ebdda7be5724a014b6a896e7325 net: reduce LOCK(cs_main) scope in ProcessGetBlockData (Andrew Toth)
613a45cd4b5482aedbdc7c61c839ea05996935c6 net: reduce LOCK(cs_main) scope in GETBLOCKTXN (Andrew Toth)
Pull request description:
Inspired by https://github.com/bitcoin/bitcoin/pull/11913 and https://github.com/bitcoin/bitcoin/pull/26308.
`cs_main` doesn't need to be locked while reading blocks. This removes the locks in `net_processing`.
ACKs for top commit:
sr-gi:
ACK [75d27fe](https://github.com/bitcoin/bitcoin/pull/26326/commits/75d27fefc7a04ebdda7be5724a014b6a896e7325)
achow101:
ACK 75d27fefc7a04ebdda7be5724a014b6a896e7325
furszy:
ACK 75d27fefc with a non-blocking nit.
mzumsande:
Code Review ACK 75d27fefc7a04ebdda7be5724a014b6a896e7325
TheCharlatan:
ACK 75d27fefc7a04ebdda7be5724a014b6a896e7325
Tree-SHA512: 79b85f748f68ecfb2f2afd3267857dd41b8e76dd482c9c922037399dcbce7b1e5d4c708a4f5fd17c3fb6699b0d88f26a17cc1d92db115dd43c8d4392ae27cec4
|
|
specific error messages
98570fe29bb08d7edc48011aa6b9731c6ab4ed2e test: add coverage for parsing cryptographically invalid pubkeys (Sebastian Falbesoner)
c740b154d193b91ca42f18759098d3fef6eaab05 rpc: use `HexToPubKey` helper for all legacy pubkey-parsing RPCs (Sebastian Falbesoner)
100e8a75bf5d8196c005331bd8f2ed42ada6d8d0 rpc: check and throw specific pubkey parsing errors in `HexToPubKey` (Sebastian Falbesoner)
Pull request description:
Parsing legacy public keys can fail for three reasons (in this order):
- pubkey is not in hex
- pubkey has an invalid length (not 33 or 65 bytes for compressed/uncompressed, respectively)
- pubkey is crytographically invalid, i.e. is not on curve (`CPubKey.IsFullyValid()` check)
Many RPCs currently perform these checks manually with different error messages, even though we already have a `HexToPubKey` helper. This PR puts all three checks in this helper (the length check was done on the call-sites before), adds specific error messages for each case, and consequently uses it for all RPCs that parse legacy pubkeys. This leads to deduplicated code and also to more consistent and detailed error messages for the user.
Affected RPC calls are `createmultisig`, `addmultisigaddress`, `importpubkey`, `importmulti`, `fundrawtransaction`, `walletcreatefundedpsbt`, `send` and `sendall`.
Note that the error code (-5 a.k.a. `RPC_INVALID_ADDRESS_OR_KEY`) doesn't change in any of the causes, so the changes are not breaking RPC API compatibility. Only the messages are more specific.
The last commits adds test coverage for the cryptographically invalid (not-on-curve) pubkey case which wasn't exercised before.
ACKs for top commit:
stratospher:
tested ACK 98570fe.
davidgumberg:
ACK https://github.com/bitcoin/bitcoin/pull/28336/commits/98570fe29bb08d7edc48011aa6b9731c6ab4ed2e
Eunovo:
Tested ACK https://github.com/bitcoin/bitcoin/pull/28336/commits/98570fe29bb08d7edc48011aa6b9731c6ab4ed2e
achow101:
ACK 98570fe29bb08d7edc48011aa6b9731c6ab4ed2e
Tree-SHA512: cfa474176e95b5b18f3a9da28fdd9e87195cd58994c1331198f2840925fff322fd323a6371feab74a1b32e4b9ea58a6dc732fa751b4cdd45402c1029af609ece
|
|
fb9f150759b22772dd48983a2be1ea397245e289 gui: fix misleading signmessage error with segwit (willcl-ark)
Pull request description:
As described in https://github.com/bitcoin/bitcoin/issues/10542 (and numerous other places), message signing in Bitcoin Core does not support "signing with a segwit address" and likely will not in the foreseeable future, or at least until a new message-signing standard is agreed upon.
Therefore update the possibly misleading error message presented to the user in the GUI to detail more specifically the reason their message cannot be signed, in the case that a non P2PKH address is entered.
This change takes the [suggested wording](https://github.com/bitcoin/bitcoin/issues/10542#issuecomment-1960313569) from @adiabat.
Perhaps with this we can close https://github.com/bitcoin/bitcoin/issues/10542 ?
ACKs for top commit:
hebasto:
ACK fb9f150759b22772dd48983a2be1ea397245e289.
Tree-SHA512: 5ba8d722ad3632dad2e0a2aa94b0f466b904e7885a247a5d26ebdfce54e3611090b103029d8dfce92adc49e50fe5f4830f687d867b4c56c3ea997e519b4e188d
|
|
keep to bitcoin-config.h includes
fa09451f8e6799682d7e7c863f25334fd1c7dce3 Add lint check for bitcoin-config.h include IWYU pragma (MarcoFalke)
dddd40ba8267dea11a3eb03d5cf8b51dbb99be5d scripted-diff: Add IWYU pragma keep to bitcoin-config.h includes (MarcoFalke)
Pull request description:
The `bitcoin-config.h` includes have issues:
* The header is incompatible with iwyu, because symbols may be defined or not defined. So the `IWYU pragma: keep` is needed to keep the include when a symbol is not defined on a platform. Compare the previous discussion in https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1948959711
* Guarding the includes by `HAVE_CONFIG_H` is verbose and brittle. Now that all build config dependencies have been removed from low level headers, the benefits are questionable, and the guard can be removed. The linter could also be tricked by guarding the include by `#if defined(HAVE_C0NFIG_H)` (`O` replaced by `0`). Compare the previous discussion in https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1483189853 .
ACKs for top commit:
achow101:
ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3
TheCharlatan:
ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3
hebasto:
re-ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/29494#pullrequestreview-2028864535) (`timedata.cpp` removed in https://github.com/bitcoin/bitcoin/pull/29623).
Tree-SHA512: 47cb973f7f24bc625acc4e78683371863675d186780236d55d886cf4130e05a78bb04f1d731aae7088313b8e963a9677cc77cf518187dbd99d776f6421ca9b52
|
|
Discover
a68fed111be393ddbbcd7451f78bc63601253ee0 net: Fix misleading comment for Discover (laanwj)
7766dd280d9a4a7ffdfcec58224d0985cfd4169b net: Replace ifname check with IFF_LOOPBACK in Discover (laanwj)
Pull request description:
Checking the interface name is kind of brittle. In the age of network namespaces and containers, there is no reason a loopback interface can't be called differently.
Check for the `IFF_LOOPBACK` flag to detect loopback interface instead.
Also remove a misleading comment in Discover's doc comment.
ACKs for top commit:
sipa:
utACK a68fed111be393ddbbcd7451f78bc63601253ee0
willcl-ark:
utACK a68fed111be393ddbbcd7451f78bc63601253ee0
theuni:
utACK a68fed111be393ddbbcd7451f78bc63601253ee0. Satoshi-era brittleness :)
Tree-SHA512: e2d7fc541f40f6a6af08286e7bcb0873ff55debdcd8b38b03f274897b673a6fb51d84d6c7241a02a9567ddf2645f50231d91bb1f55307ba7c6e68196c29b0edf
|
|
|
|
Currently, the only allowed package topology has a min size of 2.
Update the error message to reflect that.
|
|
|
|
than duplicating definition
|
|
just a single one
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](https://github.com/bitcoin/bitcoin/commit/401926283a200994ecd7df8eae8ced8e0b067c46#diff-7442c48d42cd5455a79915a0f00cce5e13359db46437a32b812876edb0a5ccddR250) (similar to how it is now).
- later on, "warnings" RPC response fields were introduced, e.g. in ef2a3de25c882396e1776b554878d2784b6b7391, with the description [stating](https://github.com/bitcoin/bitcoin/commit/ef2a3de25c882396e1776b554878d2784b6b7391#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
|
|
31a15f0aff79d2b34a9640909b9e6fb39a647b60 bench: Disable WalletCreate* benchmarks when building with MSVC (Hennadii Stepanov)
23dc0c19acd54cad1bed2f14df024b6b533f2330 msvc, bench: Add missing source files to bench_bitcoin project (Hennadii Stepanov)
Pull request description:
On the master branch, the `bench_bitcoin.vcxproj` MSVC project misses wallet-specific source files.
This PR fixes this issue.
Benchmark run on Windows:
```
> src\bench_bitcoin.exe -filter="CoinSelection|BnBExhaustion|Wallet.*"
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 398,800.00 | 2,507.52 | 1.5% | 0.01 | `BnBExhaustion`
| 584,450.00 | 1,711.01 | 1.5% | 0.01 | `CoinSelection`
| 86,603,650.00 | 11.55 | 0.4% | 1.91 | `WalletAvailableCoins`
| 7,604.00 | 131,509.73 | 0.9% | 0.01 | `WalletBalanceClean`
| 124,028.57 | 8,062.66 | 2.6% | 0.01 | `WalletBalanceDirty`
| 7,587.12 | 131,802.30 | 1.9% | 0.01 | `WalletBalanceMine`
| 48.58 | 20,583,872.99 | 0.9% | 0.01 | `WalletBalanceWatch`
| 2,371,060.00 | 421.75 | 1.3% | 0.13 | `WalletCreateTxUseOnlyPresetInputs`
| 96,861,760.00 | 10.32 | 0.9% | 5.31 | `WalletCreateTxUsePresetInputsAndCoinSelection`
| 280.71 | 3,562,424.13 | 1.5% | 0.01 | `WalletIsMineDescriptors`
| 1,033.47 | 967,618.32 | 0.3% | 0.01 | `WalletIsMineLegacy`
| 282.36 | 3,541,599.91 | 0.5% | 0.01 | `WalletIsMineMigratedDescriptors`
| 484,547,300.00 | 2.06 | 1.0% | 2.43 | `WalletLoadingDescriptors`
| 29,924,300.00 | 33.42 | 0.4% | 0.15 | `WalletLoadingLegacy`
```
ACKs for top commit:
maflcko:
lgtm ACK 31a15f0aff79d2b34a9640909b9e6fb39a647b60
Tree-SHA512: 0241af06126edf612489322cdce66ba43792066b5400b1719a8b9d1ec62030e8a9d497e2f01e38290e94c387db59ccf2a458f4b35d3dc8030a1a1413d89eb792
|
|
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."
|
|
This also changes behavior if ReadBlockFromDisk or
ReadRawBlockFromDisk fails. Previously, the node would crash
due to an assert. This has been replaced with logging the error,
disconnecting the peer, and returning early.
|
|
Also adds a static assertion that MAX_BLOCKTXN_DEPTH <= MIN_BLOCKS_TO_KEEP
|
|
63317103c9f2b0635558da814567bb79c17ae851 miniscript: make operator_mst consteval (Pieter Wuille)
Pull request description:
It seems modern compilers don't realize that all invocations of operator""_mst can be evaluated at compile time, despite the `constexpr` keyword.
Since C++20, we can force them to evaluate at compile time using `consteval`, turning all the miniscript type constants into actual compile-time constants.
This should give a nice but not very important speedup for miniscript logic, but it's also a way to start testing C++20 features.
ACKs for top commit:
hebasto:
re-ACK 63317103c9f2b0635558da814567bb79c17ae851.
theuni:
utACK 63317103c9f2b0635558da814567bb79c17ae851
Tree-SHA512: bdc9f1a6499b8bb3ca04f1a158c31e6876ba97206f95ee5718f50efd58b5b4e6b8867c07f791848430bfaa130b9676d8a68320b763cda9a340c75527acbfcc9e
|