0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-10 10:52:31 -05:00
Commit graph

26201 commits

Author SHA1 Message Date
Matthew Zipkin
cbc6c440e3
doc: add comments and release-notes for JSON-RPC 2.0 2024-05-14 11:28:48 -04:00
Matthew Zipkin
e7ee80dcf2
rpc: JSON-RPC 2.0 should not respond to "notifications"
For JSON-RPC 2.0 requests we need to distinguish between
a missing "id" field and "id":null. This is accomplished
by making the JSONRPCRequest id property a
std::optional<UniValue> with a default value of
UniValue::VNULL.

A side-effect of this change for non-2.0 requests is that request which do not
specify an "id" field will no longer return "id": null in the response.
2024-05-14 11:28:43 -04:00
Ryan Ofsky
dbb3113082
Merge bitcoin/bitcoin#30083: kernel: Remove batchpriority from kernel library
d4b17c7d46 kernel: Remove batchpriority from kernel library (TheCharlatan)

Pull request description:

  The current usage of ScheduleBatchPriority is not transparent. Once the thread scheduling is changed, it remains unchanged for the remainder of the thread's lifetime. So move the call from `ImportBlocks` to the init code where it is clearer that its effect lasts for the entire lifetime of the thread.

  Users of the kernel library might not expect `ImportBlocks` to have an influence on the thread it is called in. Particularly since it is only a compile time option and cannot be controlled at runtime. With this patch users of the kernel library can now freely choose their own scheduling policy.

  This PR is easier reviewed with `git diff --color-moved-ws=ignore-all-space --color-moved=dimmed-zebra`

  ---
  This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).

ACKs for top commit:
  maflcko:
    ACK d4b17c7d46 📭
  ryanofsky:
    Code review ACK d4b17c7d46, just added suggested comment since last review
  hebasto:
    ACK d4b17c7d46, I have reviewed the code and it looks OK.

Tree-SHA512: cafedecd9affad58ddd7f30f68bba71291ca951bb186ff4b2da04b7f21f0b26e5e3143846d032b9e391bd5ce6c7466b97aa3758d2a85ebd7353eb8b69139641a
2024-05-14 11:20:33 -04:00
Matthew Zipkin
bf1a1f1662
rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests
Avoid returning HTTP status errors for non-batch JSON-RPC 2.0 requests if the
RPC method failed but the HTTP request was otherwise valid. Batch requests
already did not return HTTP errors previously.
2024-05-14 11:15:54 -04:00
Matthew Zipkin
466b90562f
rpc: Add "jsonrpc" field and drop null "result"/"error" fields
Only for JSON-RPC 2.0 requests.
2024-05-14 10:39:43 -04:00
Matthew Zipkin
2ca1460ae3
rpc: identify JSON-RPC 2.0 requests 2024-05-14 10:32:43 -04:00
josibake
9408a04e42
tests, fuzz: use new NUMS_H const 2024-05-14 11:44:33 +02:00
glozow
0fb17bf61a [log] updates in TxOrphanage
- Add elapsed time in "remove orphan" log
- Add size in "stored orphan" log
- grammar edit
2024-05-14 10:38:57 +01:00
glozow
6675f6428d [unit test] TxOrphanage handling of same-txid-different-witness txns 2024-05-14 10:32:28 +01:00
glozow
8923edfc1f [p2p] allow entries with the same txid in TxOrphanage
Index by wtxid instead of txid to allow entries with the same txid but
different witnesses in orphanage. This prevents an attacker from
blocking a transaction from entering the orphanage by sending a mutated
version of it.
2024-05-14 10:32:28 +01:00
glozow
c31f148166 [refactor] TxOrphanage::EraseTx by wtxid
No behavior change right now, as transactions in the orphanage are
unique by txid. This makes the next commit easier to review.
2024-05-14 10:32:28 +01:00
glozow
efcc593017 [refactor] TxOrphanage::HaveTx only by wtxid 2024-05-14 10:32:27 +01:00
glozow
7e475b9648 [p2p] don't query orphanage by txid 2024-05-14 10:31:56 +01:00
TheCharlatan
d4b17c7d46
kernel: Remove batchpriority from kernel library
The current usage of ScheduleBatchPriority is not transparent. Once the
thread scheduling is changed, it remains unchanged for the remainder of
the thread's lifetime. So move the call from `ImportBlocks` to the init
code where it is clearer that its effect lasts for the entire lifetime
of the thread.

Users of the kernel library might not expect `ImportBlocks` to have an
influence on the thread it is called in. Particularly since it is only a
compile time option and cannot be controlled at runtime. With this patch
users of the kernel library can now choose their own scheduling policy.
2024-05-14 10:26:28 +02:00
josibake
b946f8a4c5
crypto: add NUMS_H const 2024-05-14 10:24:31 +02:00
Ava Chow
ecba230979 wallet: implement BerkeleyRODatabase::Backup 2024-05-13 23:01:38 -04:00
Ava Chow
0c8e728476 wallet: implement BerkeleyROBatch
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.
2024-05-13 23:01:37 -04:00
Ava Chow
756ff9b478 wallet: add dummy BerkeleyRODatabase and BerkeleyROBatch classes
BerkeleyRODatabase and BerkeleyROBatch will be used to access a BDB file
without the use of BDB. For now, these are dummy classes.
2024-05-13 23:01:06 -04:00
Sebastian Falbesoner
12d82817bf refactor: simplify FormatSubVersion using strprintf/Join
Rather than using std::ostringstream and manually joining the
comments, use strprintf and our own `Join` helper.
2024-05-13 23:31:46 +02:00
Ava Chow
d6069cb8d6
Merge bitcoin/bitcoin#28233: validation: don't clear cache on periodic flush: >2x block connection speed
4a6d1d1e3b 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.

  ![plot](https://github.com/bitcoin/bitcoin/assets/237213/802cb28d-1ad4-47c3-a886-c5366b423eca)

  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 4a6d1d1e3b
  achow101:
    ACK 4a6d1d1e3b
  brunoerg:
    crACK 4a6d1d1e3b

Tree-SHA512: 05dbc677bc309bbcf89c52a6c5e853e2816b0ef0b5ee3719b30696df315a0427e244bb82da9ad828ec0e7ea8764552f8affe14c0184b52adf1909f5d8c1b4f9e
2024-05-13 16:31:19 -04:00
Ryan Ofsky
0503cbea9a
Merge bitcoin/bitcoin#30094: rpc: move UniValue in blockToJSON
b77bad309e 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 b77bad309e
  ismaelsadeeq:
    ACK b77bad309e
  TheCharlatan:
    ACK b77bad309e
  theuni:
    utACK b77bad309e
  hebasto:
    ACK b77bad309e, I have reviewed the code and it looks OK.
  BrandonOdiwuor:
    ACK b77bad309e

Tree-SHA512: 767608331040f9cfe5c3568ed0e3c338920633472a1a50d4bbb47d1dc69d2bb11466d611f050ac8ad1a894b47fe1ea4d968cf34cbd44d4bb8d479fc5c7475f6d
2024-05-13 16:15:53 -04:00
glozow
ff8c606cf1
Merge bitcoin/bitcoin#29974: fuzz: txorphan tests fixups
58594c7040 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 58594c7040
  glozow:
    ACK 58594c7
  instagibbs:
    ACK 58594c7040

Tree-SHA512: e97cc2a43e388f87b64d2e4e45f929dd5b0dd85d668dd693b75e4c9ceea734cd7645952385d428208d07b70e1aafbec84cc2ec264a2e07d36fc8ba3e97885a8d
2024-05-13 16:00:49 +01:00
glozow
c7deb76118
Merge bitcoin/bitcoin#29994: doc: removed help text saying that peers may not connect automatically
95897ff181 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 95897ff181
  tdb3:
    ACK for 95897ff181.
  vasild:
    ACK 95897ff181
  jonatack:
    ACK 95897ff181
  kristapsk:
    ACK 95897ff181. According to https://bitnodes.io/dashboard/#user-agents stats, most nodes on the network are v23+.

Tree-SHA512: 9e35194f8a1e06f1447450af2ea30cdc43722665c2d2e4b7aa9a52afac5c1e83fed744742c836743a555cc180c90f9eebdc6637eba6190010d693eef9c5834f7
2024-05-13 15:58:36 +01:00
willcl-ark
b77bad309e
rpc: move UniValue in blockToJSON
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.
2024-05-13 14:30:44 +01:00
josibake
5676aec1e1
refactor: Model the bech32 charlimit as an Enum
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
2024-05-13 12:07:47 +02:00
merge-script
b94061902e
Merge bitcoin-core/gui#812: Fix create unsigned transaction fee bump
671b7a3251 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 671b7a3251
  hebasto:
    ACK 671b7a3251, tested on Ubuntu 24.04.

Tree-SHA512: 5b9ec5a1b91c014c05c83c63daaa8ba33f9dc1bfa930442315a0913db710df17a1b6bb4ad39f1419a7054f37ebedb7ad52e1c5d3d2fb444b1676162e89a4efd2
2024-05-12 15:30:34 +01:00
merge-script
ee9491369f
Merge bitcoin/bitcoin#29658: Bugfix: GUI: Help messages already have a trailing newline, so don't add an extra one
d1ed09a764 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 d1ed09a764, tested on Ubuntu 24.04.

Tree-SHA512: 15ee9d1060c2492bb3b04a0ac4cb53f7b959bbe32bce415793da0c221f1c963c8f2bb3996ea07d1a7c192bfc2e23be2cd7d4e5649c592eb3fc03906c2763f1aa
2024-05-12 14:21:21 +01:00
merge-script
3207286680
Merge bitcoin-core/gui#813: Don't permit port in proxy IP option
10c5275ba4 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 10c5275ba4
  hebasto:
    ACK 10c5275ba4, tested on Ubuntu 24.04.

Tree-SHA512: ed83590630cf693680a3221f701ecd18dd08710a17b726dc4978a3a6e330a34fb77d73a4f710c01bcb3faf88b6604ff37bcdbb191ce1623348ca5b92fd6fe9a7
2024-05-11 19:34:12 +01:00
merge-script
182983c6ab
Merge bitcoin-core/gui#788: debugwindow: update session ID tooltip
3bf00e1360 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

  ![sessionIDifany](https://github.com/bitcoin-core/gui/assets/93143998/d4d7df43-8281-4b1e-83fc-5a3788d7724e)

  PR

  ![sessionID](https://github.com/bitcoin-core/gui/assets/93143998/221f6831-7d12-4913-be76-325a87baad2e)

  Session ID not shown when transport v1

  ![transportv1](https://github.com/bitcoin-core/gui/assets/93143998/6c067a08-4be4-4ce1-b514-80654ca5cd43)

  <!--
  *** 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 3bf00e1360
  kristapsk:
    ACK 3bf00e1360
  jarolrod:
    ACK 3bf00e1360
  pablomartin4btc:
    tACK 3bf00e1360
  hebasto:
    ACK 3bf00e1360.

Tree-SHA512: 4de0c56c070dc5d1cee73b629bdf3d1778c6d90d512337aa6cfd3eed4ce95cbcfbe5713e2942f6fc22907b2c4d9df7979ba8e9f91f7cc173b42699ea35113f96
2024-05-11 19:13:34 +01:00
merge-script
b47c393d8a
Merge bitcoin/bitcoin#30081: refactor: Remove unused code from subprocess.h header
5a11d3023f refactor, subprocess: Remove unused stream API calls (Hennadii Stepanov)
05b6f8793c refactor, subprocess: Remove unused `Popen::child_created_` data member (Hennadii Stepanov)
9e1ccf55e1 refactor, subprocess: Remove unused `Popen::poll()` (Hennadii Stepanov)
24b53fc84a 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 5a11d3023f since it's all removals :)
  theStack:
    Code-review ACK 5a11d3023f

Tree-SHA512: 11f984f8384c337782d058afa80011e88087a1b5a3ed6e4678d492e6c232b706a26312463c5dd8c529aa477497c8afca9f54574e0e36e3edd5675bd5d8424bbb
2024-05-11 18:37:49 +08:00
Jon Atack
d0b047494c test: add GetAddedNodeInfo() CJDNS regression unit test 2024-05-10 22:29:51 -06:00
Jon Atack
684da97070 p2p, bugfix: detect addnode cjdns peers in GetAddedNodeInfo()
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
2024-05-10 22:29:51 -06:00
Ava Chow
2cedb42a92
Merge bitcoin/bitcoin#29252: kernel: Remove key module from kernel library
96378fe734 Refactor: Remove ECC_Start and ECC_Stop from key header (TheCharlatan)
41eba5bd71 kernel: Remove key module from kernel library (TheCharlatan)
a08d2b3cb9 tools: Use ECC_Context helper in bitcoin-tx and bitcoin-wallet tools (Ryan Ofsky)
28905c1a64 test: Use ECC_Context helper in bench and fuzz tests (Ryan Ofsky)
538fedde1d 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 96378fe734
  ryanofsky:
    Code review ACK 96378fe734. Just suggested comment changes since last review.
  theuni:
    utACK 96378fe734

Tree-SHA512: 40be427e8e2c920c0e3ce64a9bdd90551be27a89af11440bfb6ab0dd3a1d1ccb7cf1f82383cd782818cd1bb44d5ae5d2161cf4d78d3127ce4987342007090bab
2024-05-10 13:18:00 -04:00
Hennadii Stepanov
5a11d3023f
refactor, subprocess: Remove unused stream API calls 2024-05-10 14:58:27 +01:00
Hennadii Stepanov
05b6f8793c
refactor, subprocess: Remove unused Popen::child_created_ data member 2024-05-10 14:47:15 +01:00
Hennadii Stepanov
9e1ccf55e1
refactor, subprocess: Remove unused Popen::poll() 2024-05-10 14:47:07 +01:00
Hennadii Stepanov
24b53fc84a
refactor, subprocess: Remove Popen::pid() 2024-05-10 14:42:31 +01:00
Ava Chow
012e540ace
Merge bitcoin/bitcoin#29122: test: adds outbound eviction functional tests, updates comment in ConsiderEviction
d53d848347 test: adds outbound eviction tests for non outbound-full-relay peers (Sergi Delgado Segura)
a8d9a0edc7 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 d53d848347
  tdb3:
    Re ACK for d53d848347
  achow101:
    ACK d53d848347
  cbergqvist:
    ACK d53d848347

Tree-SHA512: 633b84bb1229fe21e2f650c1beada33ca7f190b64eafd64df2266516d21175e5d652e019ff7114f00cb8bd19f5817dc19e65adf75767a88e24dc0842ce40c63e
2024-05-09 16:20:43 -04:00
TheCharlatan
96378fe734
Refactor: Remove ECC_Start and ECC_Stop from key header
They are unused outside of the key module now.
2024-05-09 15:56:10 +02:00
TheCharlatan
41eba5bd71
kernel: Remove key module from kernel library
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`.
2024-05-09 15:56:08 +02:00
Ryan Ofsky
a08d2b3cb9
tools: Use ECC_Context helper in bitcoin-tx and bitcoin-wallet tools 2024-05-09 15:56:06 +02:00
Ryan Ofsky
28905c1a64
test: Use ECC_Context helper in bench and fuzz tests 2024-05-09 15:56:04 +02:00
Ryan Ofsky
538fedde1d
common: Add ECC_Context RAII wrapper for ECC_Start/ECC_Stop 2024-05-09 15:55:55 +02:00
Ava Chow
43003255c0
Merge bitcoin/bitcoin#29292: rpc: improve submitpackage documentation and other improvements
78e52f663f doc: rpc: fix submitpackage examples (stickies-v)
1a875d4049 rpc: update min package size error message in submitpackage (stickies-v)
f9ece258aa doc: rpc: submitpackage takes sorted array (stickies-v)
17f74512f0 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 78e52f663f
  instagibbs:
    ACK 78e52f663f
  achow101:
    ACK 78e52f663f
  glozow:
    utACK 78e52f663f

Tree-SHA512: a8845621bb1cbf784167fc7c82cb8ceb105868b65b26d3465f072d1c04ef3699e85a21a524ade805d423bcecbc34f7d5bff12f2c21cbd902ae1fb154193ebdc9
2024-05-08 18:39:56 -04:00
Martin Zumsande
0d114e3cb2 blockstorage: Add Assume for fKnown / snapshot chainstate
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.
2024-05-08 18:19:47 -04:00
Ava Chow
573f631165
Merge bitcoin/bitcoin#26326: net: don't lock cs_main while reading blocks in net processing
75d27fefc7 net: reduce LOCK(cs_main) scope in ProcessGetBlockData (Andrew Toth)
613a45cd4b 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](75d27fefc7)
  achow101:
    ACK 75d27fefc7
  furszy:
    ACK 75d27fefc with a non-blocking nit.
  mzumsande:
    Code Review ACK 75d27fefc7
  TheCharlatan:
    ACK 75d27fefc7

Tree-SHA512: 79b85f748f68ecfb2f2afd3267857dd41b8e76dd482c9c922037399dcbce7b1e5d4c708a4f5fd17c3fb6699b0d88f26a17cc1d92db115dd43c8d4392ae27cec4
2024-05-08 18:04:19 -04:00
Ava Chow
4ff42762fd
Merge bitcoin/bitcoin#28336: rpc: parse legacy pubkeys consistently with specific error messages
98570fe29b test: add coverage for parsing cryptographically invalid pubkeys (Sebastian Falbesoner)
c740b154d1 rpc: use `HexToPubKey` helper for all legacy pubkey-parsing RPCs (Sebastian Falbesoner)
100e8a75bf 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 98570fe29b
  Eunovo:
    Tested ACK 98570fe29b
  achow101:
    ACK 98570fe29b

Tree-SHA512: cfa474176e95b5b18f3a9da28fdd9e87195cd58994c1331198f2840925fff322fd323a6371feab74a1b32e4b9ea58a6dc732fa751b4cdd45402c1029af609ece
2024-05-08 17:52:58 -04:00
merge-script
4e56df8f91
Merge bitcoin-core/gui#819: Fix misleading signmessage error with segwit
fb9f150759 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 fb9f150759.

Tree-SHA512: 5ba8d722ad3632dad2e0a2aa94b0f466b904e7885a247a5d26ebdfce54e3611090b103029d8dfce92adc49e50fe5f4830f687d867b4c56c3ea997e519b4e188d
2024-05-07 21:31:14 +01:00
Ava Chow
8efd03ad04
Merge bitcoin/bitcoin#29494: build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes
fa09451f8e Add lint check for bitcoin-config.h include IWYU pragma (MarcoFalke)
dddd40ba82 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 fa09451f8e
  TheCharlatan:
    ACK fa09451f8e
  hebasto:
    re-ACK fa09451f8e, 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
2024-05-07 14:14:03 -04:00
merge-script
ef09f535b7
Merge bitcoin/bitcoin#29984: net: Replace ifname check with IFF_LOOPBACK in Discover
a68fed111b net: Fix misleading comment for Discover (laanwj)
7766dd280d 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 a68fed111b
  willcl-ark:
    utACK a68fed111b
  theuni:
    utACK a68fed111b. Satoshi-era brittleness :)

Tree-SHA512: e2d7fc541f40f6a6af08286e7bcb0873ff55debdcd8b38b03f274897b673a6fb51d84d6c7241a02a9567ddf2645f50231d91bb1f55307ba7c6e68196c29b0edf
2024-05-07 10:28:58 +08:00