a0c254c13a3ef21e257cca3493446c632b636b15 Drop CHashWriter (Anthony Towns)
c94f7e5b1cd1ddff2a7d95cfad5a83c9dfa526be Drop OverrideStream (Anthony Towns)
6e9e4e6130797b721c8df1eabaf46ec25ebb6abe Use ParamsWrapper for witness serialization (Anthony Towns)
Pull request description:
Choose whether witness is included in transaction serialization via serialization parameter rather than the stream version. See #25284 and #19477 for previous context.
ACKs for top commit:
maflcko:
re-ACK a0c254c13a3ef21e257cca3493446c632b636b15 🐜
theuni:
ACK a0c254c13a3ef21e257cca3493446c632b636b15
Tree-SHA512: 8fd5cadfd84c5128e36c34a51fb94fdccd956280e7f65b7d73c512d6a9cdb53cdd3649de99ffab5322bd34be26cb95ab4eb05932b3b9de9c11d85743f50dcb13
d799ea26edfd63434b6d1cf55500de184dc67fac doc: rewrite explanation for -par= (fanquake)
Pull request description:
The negative bound for script threads comes from the machine which generates the man pages, so may only be correct for that machine. Any other placeholder value will also be wrong for some machines. Fix this be removing the value. This also fixes help2man incorrectly bolding the value, as if it were a paramater.
Closes#28850.
ACKs for top commit:
maflcko:
lgtm ACK d799ea26edfd63434b6d1cf55500de184dc67fac
theStack:
ACK d799ea26edfd63434b6d1cf55500de184dc67fac
Tree-SHA512: 2eec0086faf4cc64bbf46b22949662f84d8546d2322c3d507fc44a4e1f64d228a2901af4fa4535c0771e3e14600be8308fc5dbd407b66ae6ae4f8878d8372c0a
1e5b86171e81ab4b022b9746bb06e1968ecf4086 test: Add test for array serialization (TheCharlatan)
d49d1988406f2f0d350bc5b552625f9823090130 refactor: Initialize magic bytes in constructor initializer (TheCharlatan)
Pull request description:
This is a followup-PR for #28423
* Initialize magic bytes in constructor
* Add a small unit test for serializing arrays.
ACKs for top commit:
sipa:
utACK 1e5b86171e81ab4b022b9746bb06e1968ecf4086
maflcko:
lgtm ACK 1e5b86171e81ab4b022b9746bb06e1968ecf4086
Tree-SHA512: 0f58d2332dc501ca9fd419f40ed4f977c83dce0169e9a0eee1ffc9f8daa2d2ef7e7df18205ba076f55d90ae6c4a20d2b51ab303150d38470a962bcc58a66f6e7
fa6b053b5c964fb35935fa994cb782c0731a56f8 mempool: persist with XOR (MarcoFalke)
Pull request description:
Currently the `mempool.dat` file stores data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan the file and move it into quarantine, or delete it, or corrupt it.
While the local wallet is expected to re-submit any pending transactions, unrelated transactions may be missing from the mempool after a restart. This may cause fee estimates to be off, or may cause block relay to be slower.
Fix this, similar to https://github.com/bitcoin/bitcoin/pull/6650, by rolling a random XOR pattern over the dat file when writing or reading it.
Obviously this can only protect against programs that accidentally and unintentionally are trying to mess with the dat file. Any program that intentionally wants to mess with the dat file can still trivially do so.
ACKs for top commit:
achow101:
re-ACK fa6b053b5c964fb35935fa994cb782c0731a56f8
glozow:
reACK fa6b053b5c964fb35935fa994cb782c0731a56f8
ismaelsadeeq:
ACK fa6b053b5c964fb35935fa994cb782c0731a56f8
Tree-SHA512: ded2ce3d81bc944b828263534e3178a1e45a914fe8e024f4a14c6561a73e301820944ecc75dd704b3d4221a7a3a5c0597ccab79546250c1197609ee981fe324e
bbbbdb0cd57d75a06357d2811363d30a498f4499 ci: Add filesystem lint check (MarcoFalke)
fada2f91108a56cc5c447bd6b6fac411e4d5cdca refactor: Replace <filesystem> with <util/fs.h> (MarcoFalke)
Pull request description:
Using `std::filesystem` is problematic:
* There is a `fs` namespace wrapper for it. So having two ways to achieve the same is confusing.
* Not using the `fs` wrapper is dangerous and buggy, because it disables known bugs by deleting problematic functions.
Fix all issues by removing use of it and adding a linter to avoid using it again in the future.
ACKs for top commit:
TheCharlatan:
ACK bbbbdb0cd57d75a06357d2811363d30a498f4499
fanquake:
ACK bbbbdb0cd57d75a06357d2811363d30a498f4499 🦀
Tree-SHA512: 0e2d49742b08eb2635e6fce41485277cb9c40fe20b81017c391d3472a43787db1278a236825714ca1e41c9d2f59913865cfb0c649e3c8ab1fb598c849f80c660
3b70f7b6156cb110c47a6e482791cf337bb6ad6d doc: fix broken doc/design/multiprocess.md links after #24352 (Ryan Ofsky)
6d43aad742c7ea28303cf2799528188938e7ce32 span: Make Span template deduction guides work in SFINAE context (Ryan Ofsky)
8062c3bdb9dd3062597ed8299e99151b612d32b7 util: Add ArgsManager SetConfigFilePath method (Ryan Ofsky)
441d00c60f0a67889d23f8556190ff99dde488bc interfaces: Rename CalculateBumpFees methods to be compatible with capn'proto (Ryan Ofsky)
156f49d682ef025fb942c997a6c5475e18eef9cf interfaces: Change getUnspentOutput return type to avoid multiprocess segfault (Ryan Ofsky)
4978754c0058bbdfbcd492f25fa49ef211e11d6e interfaces: Add schedulerMockForward method so mockscheduler RPC can work across processes (Ryan Ofsky)
924327eaf3ada45a603e80aa4a3ab38a0f8c8673 interfaces: Fix const virtual method that breaks multiprocess support (Ryan Ofsky)
82a379eca8251c736b4de6e7a2516582641ce397 streams: Add SpanReader ignore method (Russell Yanofsky)
Pull request description:
This is a collection of small changes to interfaces and code which were needed as part of multiprocess PR #10102, but have been moved here to make that PR smaller.
All of these changes are refactoring changes which do not affect behavior of current code
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
achow101:
ACK 3b70f7b6156cb110c47a6e482791cf337bb6ad6d
naumenkogs:
ACK 3b70f7b6156cb110c47a6e482791cf337bb6ad6d
maflcko:
re-ACK 3b70f7b6156cb110c47a6e482791cf337bb6ad6d 🎆
Tree-SHA512: 2368772b887056ad8a9f84c299cfde76ba45943770e3b5353130580900afa9611302195b899ced7b6e303b11f053ff204cae7c28ff4e12c55562fcc81119ba4c
The negative bound for script threads comes from the machine which
generates the man pages, so may only be correct for that machine. Any
other placeholder value will also be wrong for some machines. Fix this
be removing the value. This also fixes help2man incorrectly bolding the
value, as if it were a paramater.
Closes#28850.
fca0a8938e34cb4f6c400e1d1d0be02f027d80c5 ci: remove "--exclude banman" for fuzzing in mac (brunoerg)
f9b286353f79cdb5e55e2ff4ca47d73e14f9da48 fuzz: call lookup functions before calling `Ban` (brunoerg)
Pull request description:
Fixes#27924
To not have any discrepancy, it's required to call lookup functions before calling `Ban`. If we don't do it, the assertion `assert(banmap == banmap_read);` may fail because `BanMapFromJson` will call `LookupSubNet` and cause the discrepancy between the banned and the loaded one. It happens especially in MacOS (#27924).
Also, calling lookup functions before banning is what RPC `setban` does.
ACKs for top commit:
maflcko:
lgtm ACK fca0a8938e34cb4f6c400e1d1d0be02f027d80c5
dergoegge:
ACK fca0a8938e34cb4f6c400e1d1d0be02f027d80c5
Tree-SHA512: a3d635088a556df4507e65542157f10b41d4f87dce42927b58c3b812f262f4544b6b57f3384eef1097ffdd7c32b8dd1556aae201254960cbfbf48d45551200f7
4dd94ca18f6fc843137ffca3e6d3e97e4f19377b [refactor] remove access to mapTx in validation_block_tests (TheCharlatan)
d0cd2e804ec9b278ed9699c2ae48574b1c1613b1 [refactor] rewrite BlockAssembler inBlock and failedTx as sets of txids (glozow)
55b0939cab49d50ca5bc59105b669e379d5e7f6c scripted-diff: rename vTxHashes to txns_randomized (TheCharlatan)
a03aef9cec35b0d03aa63d7e8093f0420cd4b40b [refactor] rewrite vTxHashes as a vector of CTransactionRef (glozow)
938643c3b2b8e7b9aec1df34a2f8a95d616d8dd5 [refactor] remove access to mapTx in validation.cpp (glozow)
333367a9407701b5077e2457b1a6aa8ff5e4934b [txmempool] make CTxMemPoolEntry::lockPoints mutable (glozow)
1bf4855016e777dd8b424fe01750f9e3e97931a2 [refactor] use CheckPackageLimits for checkChainLimits (glozow)
dbc5bdbf595e9dd0330493645ebff0b8696192a3 [refactor] remove access to mapTx.find in mempool_tests.cpp (glozow)
f80909e7a31523d8f197fd650b138b9f228cd13f [refactor] remove access to mapTx in blockencodings_tests.cpp (glozow)
8892d6b744e3cbda2cf93721f573ffa7017bd898 [refactor] remove access to mapTx from rpc/mempool.cpp (glozow)
fad61aa56189f98d97af1d6f70c4eb46b8f98bf0 [refactor] get wtxid from entry instead of vTxHashes (glozow)
9cd8cafb77563243af19c95e396c2fb4fc3758df [refactor] use exists() instead of mapTx.find() (glozow)
14804699e59794e61dcfb02ff1971db96e9a06ce [refactor] remove access to mapTx from policy/rbf.cpp (glozow)
1c6a73abbd1fb773c7d0036beb952b95dde8e38b [refactor] Add helper for retrieving mempool entry (TheCharlatan)
453b4813ebc74859864803e9972b58e4be76a4d6 [refactor] Add helper for iterating through mempool entries (stickies-v)
Pull request description:
Motivation
* It seems preferable to use stdlib data structures instead of boost if they can achieve close to the same thing.
* Code external to mempool should ideally use its public helper methods instead of accessing `mapTx` or its iterators directly.
* Reduce the number of complex boost multi index type interactions
* Also see #28335 for further context/motivation. This PR together with #28385 simplifies that one.
Overview of things done in this PR:
* Make `vTxHashes` a vector of transaction references instead of a pair of transaction hash and iterator. The trade off here is that the data is retrieved on the fly with `GetEntry` instead of being cached in `vTxHashes`.
* Introduce `GetEntry` helper method to replace the more involved `GetIter` where applicable
* Replace `mapTx` access with `CTxMemPool` helper methods
* Simplify `checkChainLimits` call in `node/interfaces.cpp`
* Make `CTxMemPoolEntry`s `lockPoints`mutable such that they can be changed with a const iterator directly instead of going through `mapTx`
* Make `BlockAssembler`'s `inBlock` and `failedTx` sets of transaction hashes.
ACKs for top commit:
glozow:
reACK 4dd94ca
maflcko:
re-ACK 4dd94ca18f6fc843137ffca3e6d3e97e4f19377b 👝
stickies-v:
re-ACK 4dd94ca18f6fc843137ffca3e6d3e97e4f19377b
Tree-SHA512: c4d043f2186e4fde337591883fac66cade3058173987b49502bd65cecf69207a3df1077f6626809652ab63230013167b7f39a2b39f1c5166959e5495df57065f
Allows calling UpdateLockPoints() with a (const) txiter. Note that this
was already possible for caller using mapTx.modify(txiter). The point
here is to not be accessing mapTx when doing so.
The behavior is the same as CalculateMemPoolAncestors. The only
difference is the string returned, and the string is discarded anyway
since checkChainLimits only cares about pass/fail.
5039c346ca87d6112ea1eb124bdc622ba9e9a513 init: completely remove `-zapwallettxes` (remaining hidden option) (Sebastian Falbesoner)
Pull request description:
The `-zapwallettxes` functionality has been removed in v0.21.0 (see commit 3340dbadd38f5624642cf0e14dddbe6f83a3863b / PR #19671), with the parameter being kept as hidden option, to inform users via an exit error that `abandontransaction` should be used instead.
As any guides that still suggest to use `-zapwallettxes` would refer to a Bitcoin Core version that is EOL since many years (i.e. <= v0.20.x), it is highly unlikely that the error caused by the option is still relevant for any user, hence it seems fine to remove it now.
ACKs for top commit:
achow101:
ACK 5039c346ca87d6112ea1eb124bdc622ba9e9a513
BrandonOdiwuor:
ACK 5039c346ca87d6112ea1eb124bdc622ba9e9a513
fanquake:
ACK 5039c346ca87d6112ea1eb124bdc622ba9e9a513
Tree-SHA512: e3ccc6918e0f8fa68dbd1a7ec4999cc2a44e28038711919fcddaf0727648c73a9ba0fb77674317147592a113fad20755d4e727f48176bc17b048fbdebad2d6c9
fabb5046a7365af3079e6e45606d63576bc6ad12 fuzz: Avoid timeout and bloat in fuzz targets (MarcoFalke)
Pull request description:
If the fuzz input contains invalid data *in a loop*, abort early. This will teach the fuzz engine to look for useful data and avoids bloating the fuzz input folder with useless (repeated) data.
ACKs for top commit:
dergoegge:
utACK fabb5046a7365af3079e6e45606d63576bc6ad12
brunoerg:
crACK fabb5046a7365af3079e6e45606d63576bc6ad12
Tree-SHA512: 26da100d7558ae6fdd5292fb146d8858b2af8f78c546ca2509b9d27b33a33e9462ecb6035de142f9f36dd5de32f8cbad099d6c7a697902d23e1bb621cd27dc88
0420f99f429ce2382057e101859067f40de47be0 Create net_peer_connection unit tests (Jon Atack)
4b834f649921aceb44d3e0b5a2ffd7847903f9f7 Allow unit tests to access additional CConnman members (Jon Atack)
34b9ef443bc2655a85c8802edc5d5d48d792a286 net/rpc: Makes CConnman::GetAddedNodeInfo able to return only non-connected address on request (Sergi Delgado Segura)
94e8882d820969ddc83f24f4cbe1515a886da4ea rpc: Prevents adding the same ip more than once when formatted differently (Sergi Delgado Segura)
2574b7e177ef045e64f1dd48cb000640ff5103d3 net/rpc: Check all resolved addresses in ConnectNode rather than just one (Sergi Delgado Segura)
Pull request description:
## Rationale
Currently, `addnode` has a couple of corner cases that allow it to either connect to the same peer more than once, hence wasting outbound connection slots, or add redundant information to `m_added_nodes`, hence making Bitcoin iterate through useless data on a regular basis.
### Connecting to the same node more than once
In general, connecting to the same node more than once is something we should try to prevent. Currently, this is possible via `addnode` in two different ways:
1. Calling `addnode` more than once in a short time period, using two equivalent but distinct addresses
2. Calling `addnode add` using an IP, and `addnode onetry` after with an address that resolved to the same IP
For the former, the issue boils down to `CConnman::ThreadOpenAddedConnections` calling `CConnman::GetAddedNodeInfo` once, and iterating over the result to open connections (`CConman::OpenNetworkConnection`) on the same loop for all addresses.`CConnman::ConnectNode` only checks a single address, at random, when resolving from a hostname, and uses it to check whether we are already connected to it.
An example to test this would be calling:
```
bitcoin-cli addnode "127.0.0.1:port" add
bitcoin-cli addnode "localhost:port" add
```
And check how it allows us to perform both connections some times, and some times it fails.
The latter boils down to the same issue, but takes advantage of `onetry` bypassing the `CConnman::ThreadOpenAddedConnections` logic and calling `CConnman::OpenNetworkConnection` straightaway. A way to test this would be:
```
bitcoin-cli addnode "127.0.0.1:port" add
bitcoin-cli addnode "localhost:port" onetry
```
### Adding the same peer with two different, yet equivalent, addresses
The current implementation of `addnode` is pretty naive when checking what data is added to `m_added_nodes`. Given the collection stores strings, the checks at `CConnman::AddNode()` basically check wether the exact provided string is already in the collection. If so, the data is rejected, otherwise, it is accepted. However, ips can be formatted in several ways that would bypass those checks.
Two examples would be `127.0.0.1` being equal to `127.1` and `[::1]` being equal to `[0:0:0:0:0:0:0:1]`. Adding any pair of these will be allowed by the rpc command, and both will be reported as connected by `getaddednodeinfo`, given they map to the same `CService`.
This is less severe than the previous issue, since even tough both nodes are reported as connected by `getaddednodeinfo`, there is only a single connection to them (as properly reported by `getpeerinfo`). However, this adds redundant data to `m_added_nodes`, which is undesirable.
### Parametrize `CConnman::GetAddedNodeInfo`
Finally, this PR also parametrizes `CConnman::GetAddedNodeInfo` so it returns either all added nodes info, or only info about the nodes we are **not** connected to. This method is used both for `rpc`, in `getaddednodeinfo`, in which we are reporting all data to the user, so the former applies, and to check what nodes we are not connected to, in `CConnman::ThreadOpenAddedConnections`, in which we are currently returning more data than needed and then actively filtering using `CService.fConnected()`
ACKs for top commit:
jonatack:
re-ACK 0420f99f429ce2382057e101859067f40de47be0
kashifs:
> > tACK [0420f9](0420f99f42)
sr-gi:
> > > tACK [0420f9](0420f99f42)
mzumsande:
Tested ACK 0420f99f429ce2382057e101859067f40de47be0
Tree-SHA512: a3a10e748c12d98d439dfb193c75bc8d9486717cda5f41560f5c0ace1baef523d001d5e7eabac9fa466a9159a30bb925cc1327c2d6c4efb89dcaf54e176d1752
1147e00e59e47f27024ec96629993c66a3ce4ef0 [validation] change package-fee-too-low, return wtxid(s) and effective feerate (glozow)
10dd9f2441f4618321bfa2865449ac2223c572a0 [test] use CheckPackageMempoolAcceptResult in previous tests (glozow)
3979f1afcbef5fdd3fad56312573a6733a7d78a4 [validation] add TxValidationResult::TX_RECONSIDERABLE, TX_UNKNOWN (glozow)
5c786a026aee434363ad54f4346211d0e2c5a38d [refactor] use Wtxid for m_wtxids_fee_calculations (glozow)
Pull request description:
Split off from #26711 (suggested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253). This is part of #27463.
- Add 2 new TxValidationResults
- `TX_RECONSIDERABLE` helps us encode transactions who have failed fee checks that can be bypassed using package validation. This is distinguished from `TX_MEMPOOL_POLICY` so that we re-validate a transaction if and only if it is eligible for package CPFP. In the future, we will have a separate cache for reconsiderable rejects so these transactions don't go in `m_recent_rejects`.
- `TX_UNKNOWN` helps us communicate that we aborted package validation and didn't finish looking at this transaction: it's not valid but it's also not invalid (i.e. don't cache it as a rejected tx)
- Return effective feerate and the wtxids of transactions used to calculate that effective feerate when the error is `TX_SINGLE_FAILURE`. Previously, we would only provide this information if the transaction passed. Now that we have package validation, it's much more helpful to the caller to know how the failing feerate was calculated. This can also be used to improve our submitpackage RPC result (which is currently a bit unhelpful when things fail).
- Use the newly added `CheckPackageMempoolAcceptResult` for existing package validation tests. This increases test coverage and helps test the changes made in this PR.
ACKs for top commit:
instagibbs:
reACK 1147e00e59
achow101:
ACK 1147e00e59e47f27024ec96629993c66a3ce4ef0
murchandamus:
reACK 1147e00e59e47f27024ec96629993c66a3ce4ef0
ismaelsadeeq:
ACK 1147e00e59e47f27024ec96629993c66a3ce4ef0
Tree-SHA512: ac1cd73c2b487a1b99d329875d39d8107c91345a5b0b241d54a6a4de67faf11be69a2721cc732c503024a9cca381dac33d61e187957279e3c82653bea118ba91
df69b22f2e3cc03764a582f29a16a36114f67e17 doc: improve documentation around connection limit maximums (Amiti Uttarwar)
adc171edf45ec90857d990b8ec570f3c8c2242b7 scripted-diff: Rename connection limit variables (Amiti Uttarwar)
e9fd9c0225527ec7727d2a7ccbdf028784aadc6c net: add m_max_inbound to connman (Amiti Uttarwar)
c25e0e05550426f29d79571368d90f63fb472b02 net, refactor: move calculations for connection type limits into connman (Amiti Uttarwar)
Pull request description:
This is joint work with amitiuttarwar.
This has the first few commits of #28463. It is not strictly a prerequisite for that, but has changes that in our opinion make sense on their own.
It improves the handling of maximum numbers for different connection types (that are set during init and don’t change after) by:
* moving all calculations into one place, `CConnMan::Init()`. Before, they were dispersed between `Init`, `CConnman::Init` and other parts of `CConnman`, resulting in some duplicated test code.
* removing the possibility of having a negative maximum of inbound connections, which is hard to argue about
* renaming of variables and doc improvements
ACKs for top commit:
amitiuttarwar:
co-author review ACK df69b22f2e3cc03764a582f29a16a36114f67e17
naumenkogs:
ACK df69b22f2e3cc03764a582f29a16a36114f67e17
achow101:
ACK df69b22f2e3cc03764a582f29a16a36114f67e17
Tree-SHA512: 913d56136bc1df739978de50db67302f88bac2a9d34748ae96763288d97093e998fc0f94f9b6eff12867712d7e86225af6128f4170bf2b5b8ab76f024870a22c
c1144f0076339c775f41d4b5fcfdc72191440d96 tests: Reset node context members on ~BasicTestingSetup (TheCharlatan)
9759af17ff7c28eb909cb73f62b78c90851ab74d shutdown: Destroy kernel last (TheCharlatan)
Pull request description:
The destruction/resetting of node context members in the tests should roughly follow the behavior of the `Shutdown` function in `init.cpp`.
This was originally requested by MarcoFalke in this [comment](https://github.com/bitcoin/bitcoin/pull/25065#discussion_r890161249) in response to the [original pull request](https://github.com/bitcoin/bitcoin/pull/25065) introducing the `kernel::Context`.
ACKs for top commit:
maflcko:
ACK c1144f0076339c775f41d4b5fcfdc72191440d96 🗣
achow101:
ACK c1144f0076339c775f41d4b5fcfdc72191440d96
ryanofsky:
Code review ACK c1144f0076339c775f41d4b5fcfdc72191440d96. No code changes since last review, just updated commits and descriptions
Tree-SHA512: 819bb85ff82a5c6c60e429674d5684f3692fe9062500d00a87b361cc59e6bda145be21b5a4466dee6791faed910cbde4d26baab325bf6daa1813af13a63588ff
aee5404e02e203a256c1a97b629b9b107cc8bb07 Add support for RNDR/RNDRRS for aarch64 on Linux (John Moffett)
Pull request description:
This checks whether the ARMv8.5-A optional TRNG extensions [RNDR](https://developer.arm.com/documentation/ddi0601/2022-12/AArch64-Registers/RNDR--Random-Number) and [RNDRRS](https://developer.arm.com/documentation/ddi0601/2022-12/AArch64-Registers/RNDRRS--Reseeded-Random-Number) are available and, if they are, uses them for random entropy purposes.
They are nearly functionally identical to the x86 RDRAND/RDSEED extensions and are used in a similar manner.
Currently, there [appears to be](https://marcin.juszkiewicz.com.pl/download/tables/arm-socs.html) only one actual hardware implementation -- the Amazon Graviton 3. (See the `rnd` column in the link.) However, future hardware implementations may become available.
It's not possible to directly query for the capability in userspace, but the Linux kernel [added support](1a50ec0b3b) for querying the extension via `getauxval` in version 5.6 (in 2020), so this is limited to Linux-only for now.
Reviewers may want to launch any of the `c7g` instances from AWS to test the Graviton 3 hardware. Alternatively, QEMU emulates these opcodes for `aarch64` with CPU setting `max`.
Output from Graviton 3 hardware:
```
ubuntu@ip:~/bitcoin$ src/bitcoind -regtest
2023-01-06T20:01:48Z Bitcoin Core version v24.99.0-3670266ce89a (release build)
2023-01-06T20:01:48Z Using the 'arm_shani(1way,2way)' SHA256 implementation
2023-01-06T20:01:48Z Using RNDR and RNDRRS as additional entropy sources
2023-01-06T20:01:48Z Default data directory /home/ubuntu/.bitcoin
```
Graviton 2 (doesn't support extensions):
```
ubuntu@ip:~/bitcoin$ src/bitcoind -regtest
2023-01-06T20:05:04Z Bitcoin Core version v24.99.0-3670266ce89a (release build)
2023-01-06T20:05:04Z Using the 'arm_shani(1way,2way)' SHA256 implementation
2023-01-06T20:05:04Z Default data directory /home/ubuntu/.bitcoin
```
This partially closes#26796. As noted in that issue, OpenSSL [added support](https://github.com/openssl/openssl/pull/15361) for these extensions a little over a year ago.
ACKs for top commit:
achow101:
ACK aee5404e02e203a256c1a97b629b9b107cc8bb07
laanwj:
Tested ACK aee5404e02e203a256c1a97b629b9b107cc8bb07
Tree-SHA512: 1c1eb345d6690f5307a87e9bac8f06a0d1fdc7ca35db38fa22192510a44289a03252e4677dc7cbf731a27e6e3a9a4e42b6eb4149fe063bc1c905eb2536cdb1d3
bbb68ffdbdafb6717dcadac074f6098750b8aa77 refactor: drop protocol.h include header in rpc/util.h (Jon Atack)
1dd62c5295ca319c94f7233bcb2e11f9d37a33f1 refactor: move GetServicesNames from rpc/util.{h,cpp} to rpc/net.cpp (Jon Atack)
Pull request description:
Move `GetServicesNames()` from `rpc/util` to `rpc/net.cpp`, as it is only called from that compilation unit and there is no reason for other ones to need it.
Remove the `protocol.h` include in `rpc/util.h`, as it was only needed for `GetServicesNames()`, drop an unneeded forward declaration (the other IWYU suggestions would require more extensive changes in other files), and add 3 already-missing include headers in other translation units that are needed to compile without `protocol.h` in `rpc/util.h`, as `protocol.h` includes `netaddress.h`, which in turn includes `util/strencodings.h`.
ACKs for top commit:
kevkevinpal:
lgtm ACK [bbb68ff](bbb68ffdbd)
ns-xvrn:
ACK bbb68ff
achow101:
ACK bbb68ffdbdafb6717dcadac074f6098750b8aa77
Tree-SHA512: fcbe195874dd4aa9e86548685b6b28595a2c46f9869b79b6e2b3835f76b49cab4bef6a59c8ad6428063a41b7bb6f687229b06ea614fbd103e0531104af7de55d
af0fca530e4d8311bcb24a14c416e5ad7c30ff78 netbase: use reliable send() during SOCKS5 handshake (Vasil Dimov)
1b19d1117ca5373a15313227b547ef4392022dbd sock: change Sock::SendComplete() to take Span (Vasil Dimov)
Pull request description:
The `Socks5()` function which does the SOCKS5 handshake with the SOCKS5 proxy sends bytes to the socket without retrying partial writes.
`send(2)` may write only part of the provided data and return. In this case the caller is responsible for retrying the operation with the remaining data. Change `Socks5()` to do that. There is already a method `Sock::SendComplete()` which does exactly that, so use it in `Socks5()`.
A minor complication for this PR is that `Sock::SendComplete()` takes `std::string` argument whereas `Socks5()` has `std::vector<uint8_t>`. Thus the necessity for the first commit. It is possible to do also in other ways - convert the data in `Socks5()` to `std::string` or have just one `Sock::SendComplete()` that takes `void*` and change the callers to pass `str.data(), str.size()` or `vec.data(), vec.size()`.
This came up while testing https://github.com/bitcoin/bitcoin/pull/27375.
ACKs for top commit:
achow101:
ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78
jonatack:
ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78
pinheadmz:
ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78
Tree-SHA512: 1d4a53d0628f7607378038ac56dc3b8624ce9322b034c9547a0c3ce052eafb4b18213f258aa3b57bcb4d990a5e0548a37ec70af2bd55f6e8e6399936f1ce047a
f06016d77d848133fc6568f287bb86b644c9fa69 wallet: Add asserts to detect unset transaction height values (Ryan Ofsky)
262a78b13365e5318741187fde431c4974539494 wallet, refactor: Add CWalletTx::updateState function (Ryan Ofsky)
Pull request description:
Originally, this PR fixed a wallet migration bug that could cause the watchonly wallet created by legacy wallet migration to have incorrect transaction height values. A different fix for the bug was implemented in #28609, but that PR did not add any test coverage that would have caught the bug, and didn't include other changes from this PR intended to prevent problems from invalid transaction heights.
This PR adds new asserts to catch invalid transaction heights, which would trigger test failures without bugfix in #28609. This PR also refactors code and adds comments to clarify assumptions and make it less likely a bug from invalid transaction height values would be introduced.
ACKs for top commit:
achow101:
ACK f06016d77d848133fc6568f287bb86b644c9fa69
Sjors:
utACK f06016d77d848133fc6568f287bb86b644c9fa69
furszy:
Code review ACK f06016d
Tree-SHA512: 82657c403724d60354f7676b53bcfcc95bdc5864e051a2eb8bfad09d8ad35615393b2d6b432b46f908def9be37bebded3a55ec9ae19e19371d35897fe842c92e
With subpackage evaluation and de-duplication, it's not always the
entire package that is used in CheckFeerate. To be more helpful to the
caller, specify which transactions were included in the evaluation and
what the feerate was.
Instead of PCKG_POLICY (which is supposed to be for package-wide
errors), use PCKG_TX.
fa7ba926300f44b350d04262e569cc607e4991d8 fuzz: Avoid utxo_total_supply timeout (MarcoFalke)
Pull request description:
Looks like this still may take a long time to run large fuzz inputs. Thus, reduce it further, but still allow it to catch the regression, if re-introduced:
```diff
diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp
index f949655909..4bdd15c5ee 100644
--- a/src/consensus/tx_check.cpp
+++ b/src/consensus/tx_check.cpp
@@ -40,7 +40,7 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state)
std::set<COutPoint> vInOutPoints;
for (const auto& txin : tx.vin) {
if (!vInOutPoints.insert(txin.prevout).second)
- return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate");
+ {}//return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate");
}
if (tx.IsCoinBase())
```
This is the second take, see https://github.com/bitcoin/bitcoin/pull/27780. If in the future it still times out, I think the fuzz test can just be removed.
Example input:
```
JREROy5pcnAgQyw7IC4ODg4ODg4ODg4O0dEODg4ODg4ZDg4ODg4ODg4ODg7RDg4ODg4ODg4O0dEODg4ODg4ODg4ODg7R0Q4ODg4ODg4ODtHRDg4ODtHR0dEODg4O0dEODg7R0Q4ODg4ODg4ODtHRDg4ODg4ODg4ODg4O0dEODg4ODg4ODg7R0Q4ODg7R0Q4O0dEODg4ODg4ODg4ODg7R0Q4ODg4ODtHRDg4ODtHR
ACKs for top commit:
dergoegge:
ACK fa7ba926300f44b350d04262e569cc607e4991d8
brunoerg:
utACK fa7ba926300f44b350d04262e569cc607e4991d8
Tree-SHA512: 154a4895834babede6ce7b775562a7026637af1097e53e55676e92f6cf966ae0c092300ebf7e51a397eebd11f7b41d020586663e781f70d084efda1c0fe851b4
5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7 test: remove custom rpc timeout for `wallet_miniscript.py`, reorder in test_runner (Sebastian Falbesoner)
f811a24421fe102a96ab8f75427cc6a3c5503dc3 wallet: cache descriptor ID to avoid repeated descriptor string creation (Sebastian Falbesoner)
Pull request description:
Right now a wallet descriptor is converted to its string representation (via `Descriptor::ToString`) repeatedly at different instances:
- on finding a `DescriptorScriptPubKeyMan` for a given descriptor (`CWallet::GetDescriptorScriptPubKeyMan`, e.g. used by the `importdescriptors` RPC); the string representation is created once for each spkm in the wallet and at each iteration again for the searched descriptor (`DescriptorScriptPubKeyMan::HasWalletDescriptor`)
- whenever `DescriptorScriptPubKeyMan::GetID()` is called, e.g. in `TopUp` or any instances where a descriptor is written to the DB to determine the database key, also at less obvious places like `FastWalletRescanFilter` etc.
As there is no good reason to calculate a fixed descriptor's string/ID more than once, add the ID as a field to `WalletDescriptor` and calculate it immediately at initialization (or deserialization). `HasWalletDescriptor` is changed to compare the spkm's and searched descriptor's ID instead of the string to take use of that.
This speeds up the functional test `wallet_miniscript.py` by a factor of 5-6x on my machine (3m30.95s on master vs. 0m38.02s on PR). The recently introduced "max-size TapMiniscript" test-case introduced a descriptor that takes 2-3 seconds to create a string representation, so the repeated calls to that were significantly hurting the performance.
Fixes https://github.com/bitcoin/bitcoin/issues/28800.
ACKs for top commit:
Sjors:
ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
S3RK:
Code Review ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
achow101:
ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
BrandonOdiwuor:
ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
Tree-SHA512: 98b43963a5dde6055bb26cecd3b878dadd837d6226af4c84142383310495da80b3c4bd552e73b9107f2f2ff1c11f5e18060c6fd3d9e44bbd5224114c4d245c1c
