Age | Commit message (Collapse) | Author |
|
copyable
705e3f1de00bf30d728addd52a790a139d948e32 refactor: Make CTxMemPoolEntry only explicitly copyable (TheCharlatan)
Pull request description:
This has the goal of prohibiting users from accidentally creating runtime failures, e.g. by interacting with iterator_to with a copied entry. This was brought up here: https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1814794954.
CTxMemPoolEntry is already implicitly not move-constructable. So be explicit about this and use a std::list to collect the values in the policy_estimator fuzz test instead of a std::vector.
ACKs for top commit:
maflcko:
ACK 705e3f1de00bf30d728addd52a790a139d948e32 🌯
achow101:
ACK 705e3f1de00bf30d728addd52a790a139d948e32
ajtowns:
ACK 705e3f1de00bf30d728addd52a790a139d948e32
ismaelsadeeq:
ACK 705e3f1de00bf30d728addd52a790a139d948e32
Tree-SHA512: 62056905c679c919d00f9ae065ed66ac986e7e7062015aea542843d8deecda57104d7a68d002f7b20afa3164f8e9215d2d2d002c167224129540e3b1bd0712cc
|
|
nVersion
fae76a1f2ac0b352cdd708e6d57f8605bf40112e scripted-diff: Use DataStream in most places (MarcoFalke)
fac39b56b723f79ba0aa9570708a1d4a600f4e12 refactor: SpanReader without nVersion (MarcoFalke)
Pull request description:
The serialize version is unused, so remove it. This also allows to remove `GCS_SER_VERSION` and allows a scripted-diff to remove most of `CDataStream`.
ACKs for top commit:
ajtowns:
ACK fae76a1f2ac0b352cdd708e6d57f8605bf40112e
ryanofsky:
Code review ACK fae76a1f2ac0b352cdd708e6d57f8605bf40112e
Tree-SHA512: 3b487dba8ea380f1eacff9fdfb9197f025dbc30906813d3f4c3e6f1e9e4d9f2a169c6f163f51d135e18af538be78e2d2b13d694073ad25c5762980ae971a4c83
|
|
|
|
fa825975b57bc5c4323e5d6f4bc8cd2b8189e238 fuzz: Avoid timeout in process_messages (MarcoFalke)
Pull request description:
Reduce the number of messages per fuzz input. There should be no reason to have more messages than that.
This should also avoid timeouts, such as https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64548. CC https://github.com/bitcoin/bitcoin/issues/28812
ACKs for top commit:
dergoegge:
utACK fa825975b57bc5c4323e5d6f4bc8cd2b8189e238
Tree-SHA512: eeff732f7b0bd9a71f23aeecbf813d31fe34d355b906fd0384a43075cbc3cebc46a26df741b0f337208d8b33b3e28210c9b9437e2eed77844f03131bb8f5f2a1
|
|
The field is unused, so remove it.
This is also required for future commits.
|
|
version and type
fa79a881ce0537e1d74da296a7513730438d2a02 refactor: P2P transport without serialize version and type (MarcoFalke)
fa9b5f4fe32c0cfe2e477bb11912756f84a52cfe refactor: NetMsg::Make() without nVersion (MarcoFalke)
66669da4a5ca9edf2a40d20879d9a8aaf2b9e2ee Remove unused Make() overload in netmessagemaker.h (MarcoFalke)
fa0ed0794161d937d2d3385963c1aa5624b60d17 refactor: VectorWriter without nVersion (MarcoFalke)
Pull request description:
Now that the serialize framework ignores the serialize version and serialize type, everything related to it can be removed from the code.
This is the first step, removing dead code from the P2P stack. A different pull will remove it from the wallet and other parts.
ACKs for top commit:
ajtowns:
reACK fa79a881ce0537e1d74da296a7513730438d2a02
Tree-SHA512: 785b413580d980f51f0d4f70ea5e0a99ce14cd12cb065393de2f5254891be94a14f4266110c8b87bd2dbc37467676655bce13bdb295ab139749fcd8b61bd5110
|
|
|
|
Currently the damaging of input data for decryption (either ciphertext
or aad) only ever happens in the lower nibble within the byte at the
damage position, as the bit position for the `damage_val` byte was
calculated with `damage_bit & 3` (corresponding to `% 4`) rather than
`damage_bit & 7` (corresponding to the expected `% 8`).
|
|
faf1fb207fb6e9a12c864074f8c40d5922d93ff4 Fix IWYU for the script_flags fuzz target (MarcoFalke)
fa71285b7301b2993bcc68525649716afbd9abf8 fuzz: Limit fuzz buffer size in script_flags target (MarcoFalke)
fa6b87b9ee661d8ef4ec244d230ebdeb7d1841a0 fuzz: CDataStream -> DataStream in script_flags (MarcoFalke)
Pull request description:
Most fuzz targets have an upper limit on the buffer size to avoid excessive runtime. Do the same for `script_flags` to avoid timeouts such as https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1824696971
Also, fix iwyu. Also, remove legacy `CDataStream`.
ACKs for top commit:
dergoegge:
ACK faf1fb207fb6e9a12c864074f8c40d5922d93ff4
brunoerg:
utACK faf1fb207fb6e9a12c864074f8c40d5922d93ff4
Tree-SHA512: 9301917b353f7409e448b6fd3635de19330856e0742431db5ef04e62873501b5b4cd6cb78ad81ada2747fa2bdae033115b5951d10489dd5d0d320426c8b96bee
|
|
9e58c5bcd96e7ff2062274868814ccae0626589e Use Txid in COutpoint (dergoegge)
Pull request description:
This PR changes the type of the hash of a transaction outpoint from `uint256` to `Txid`.
ACKs for top commit:
Sjors:
ACK 9e58c5bcd96e7ff2062274868814ccae0626589e
stickies-v:
ACK 9e58c5bcd96e7ff2062274868814ccae0626589e. A sizeable diff, but very straightforward changes. Didn't see anything controversial. Left a few nits, but nothing blocking, only if you have to retouch.
TheCharlatan:
ACK 9e58c5bcd96e7ff2062274868814ccae0626589e
Tree-SHA512: 58f61ce1c58668f689513e62072a7775419c4d5af8f607669cd8cdc2e7be9645ba14af7f9e2d65da2670da3ec1ce7fc2a744037520caf799aba212fd1ac44b34
|
|
47e5c9994c087d1826ccc0d539e916845b5648fb fuzz: add target for `DescriptorScriptPubKeyMan` (brunoerg)
641dddf01812407d163520def83f5975413691e4 fuzz: create ConsumeCoins (brunoerg)
2e1833ca1341ab4dc92508a59181aa6c7c38db88 fuzz: move `MockedDescriptorConverter` to `fuzz/util` (brunoerg)
Pull request description:
This PR adds fuzz target for `DescriptorScriptPubKeyMan`. Also, moves `MockedDescriptorConverter` to `fuzz/util/descriptor` to be used here and in `descriptor` target.
ACKs for top commit:
maflcko:
lgtm ACK 47e5c9994c087d1826ccc0d539e916845b5648fb 🏓
dergoegge:
ACK 47e5c9994c087d1826ccc0d539e916845b5648fb
Tree-SHA512: 519acca6d7b7a3a0bfc031441b02d5980b12bfb97198bd1958a83cd815ceb9eb1499a48a3f0a7fe20e5d06d83b89335d987376fc0a014e2106b0bc0e9838dd02
|
|
Also, export script_error.h from interpreter.h, because there should
rarely be a case where script_error.h is included without interpreter.h
|
|
|
|
|
|
|
|
notifications
`CBlockPolicyEstimator` will implement `CValidationInterface` and
subscribe to its notification to process transactions added and removed
from the mempool.
Re-delegate calculation of `validForFeeEstimation` from validation to fee estimator.
Also clean up the validForFeeEstimation arg thats no longer needed in `CTxMempool`.
Co-authored-by: Matt Corallo <git@bluematt.me>
|
|
Create a new struct `NewMempoolTransactionInfo` that will be used as the new parameter of
`TransactionAddedToMempool` callback.
|
|
Update `processBlock` parameter to reference to a vector of `RemovedMempoolTransactionInfo`.
|
|
|
|
|
|
|
|
The nVersion field is unused, so remove it.
This is also required for future commits.
Also, add PushMessage aliases in PeerManagerImpl to make calling code
less verbose.
Co-Authored-By: Anthony Towns <aj@erisian.com.au>
|
|
This has the goal of prohibiting users from accidentally creating
runtime failures, e.g. by interacting with iterator_to with a copied
entry.
CTxMemPoolEntry is already implicitly not move-constructable. So be
explicit about this and use a std::list to collect the values in the
policy_estimator fuzz test instead of a std::vector.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
|
|
Also bump includes per suggestions from iwyu.
|
|
|
|
The field is unused, so remove it.
This is also required for future commits.
|
|
83986f464c59a6517f790a960a72574e167f3f72 Include version.h in fewer places (Anthony Towns)
c7b61fd61b199cbefda660c9d394bb4035a49528 Convert some CDataStream to DataStream (Anthony Towns)
1410d300df7e57a895f2697d9849a2201021c973 serialize: Drop useless version param from GetSerializeSize() (Anthony Towns)
bf574a75016123309b894da895ab1c7a81731933 serialize: drop GetSerializeSizeMany (Anthony Towns)
efa9eb6d7c8012fe4ed85699d81c8fe5dd18da1e serialize: Drop nVersion from [C]SizeComputer (Anthony Towns)
Pull request description:
Drops the version field from `GetSerializeSize()`, simplifying the code in various places. Also drop `GetSerializeSizeMany()` (as just removing the version parameter could result in silent bugs) and remove unnecessary instances of `#include <version.h>`.
ACKs for top commit:
maflcko:
ACK 83986f464c59a6517f790a960a72574e167f3f72 📒
theuni:
ACK 83986f464c59a6517f790a960a72574e167f3f72.
Tree-SHA512: 36617b6dfbb1b4b0afbf673e905525fc6d623d3f568d3f86e3b9d4f69820db97d099e83a88007bfff881f731ddca6755ebf1549e8d8a7762437dfadbf434c62e
|
|
faa25718b3f11f24aa41f0968bbd4da104814bc5 fuzz: AutoFile with XOR (MarcoFalke)
fab5cb9066366d93531f34e649a10addf44cd2ca fuzz: Reduce LIMITED_WHILE limit for file fuzzing (MarcoFalke)
fa5388fad3e87d56395bfe2467d2d6448a8f2e40 fuzz: Remove FuzzedAutoFileProvider (MarcoFalke)
Pull request description:
This should help to get fuzz coverage for https://maflcko.github.io/b-c-cov/fuzz.coverage/src/streams.cpp.gcov.html
Also, remove unused code and fix a timeout bug.
ACKs for top commit:
dergoegge:
ACK faa25718b3f11f24aa41f0968bbd4da104814bc5
Tree-SHA512: 56f1e6fd5cb2b66ffd9a7d9c09c9b8e396be3e7485feb03b35b6bd3c48e624fdaed50b472e4ffec21f09efb5e949d7ee32a13851849c9140b6b4cf25917dd7ac
|
|
43de4d3630274e1287179c86896ed4c2d8b9eff4 doc: fix typos (Sjors Provoost)
Pull request description:
This PR fixes typos found by lint-spelling.py using codespell 2.2.6.
Our CI linter job uses codespell 2.2.5 and found fewer typos that I did locally. In any case it's happy now.
ACKs for top commit:
pablomartin4btc:
re ACK 43de4d3630274e1287179c86896ed4c2d8b9eff4
Tree-SHA512: c032fe86cb49c924a468385653b31f309a9db68c478d70335bba3e65a1ff3826abe80284fe00a090ab5a509e1edbf17e476f6922fb15d055e50f1103dad2ccb0
|
|
6a917918b76eef154c6757fe9ecf7713d526c3dd fuzz: allow fake and duplicate inputs in tx_package_eval target (Greg Sanders)
a0626ccdadc0e965dc818d8a7c862e8c81b54fd1 fuzz: allow reaching MempoolAcceptResult::ResultType::DIFFERENT_WITNESS in tx_package_eval target (Greg Sanders)
Pull request description:
Exercises `DIFFERENT_WITNESS` by using "blank" WSH() and allowing witness to determine wtxid, and attempts to make invalid/duplicate inputs.
ACKs for top commit:
dergoegge:
Coverage looks good to me ACK 6a917918b76eef154c6757fe9ecf7713d526c3dd
Tree-SHA512: db894f5f5b81c6b454874baf11f296462832285f41ccb09f23c0db92b9abc98f8ecacd72fc8f60dc92cb7947f543a2e55bed2fd210b0e8ca7c7d5389d90b14af
|
|
|
|
|
|
A higher limit is not needed, and only leads to timeouts, see for
example the buffered_file one in
https://github.com/bitcoin/bitcoin/issues/28812#issue-1981386486
|
|
The code is clearer without it.
This is also needed for a future commit.
|
|
|
|
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
|
|
|
|
tx_package_eval target
|
|
Also, compare banmaps only if there are no invalid
entries.
|
|
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](https://github.com/bitcoin/bitcoin/commit/0420f99f429ce2382057e101859067f40de47be0)
sr-gi:
> > > tACK [0420f9](https://github.com/bitcoin/bitcoin/commit/0420f99f429ce2382057e101859067f40de47be0)
mzumsande:
Tested ACK 0420f99f429ce2382057e101859067f40de47be0
Tree-SHA512: a3a10e748c12d98d439dfb193c75bc8d9486717cda5f41560f5c0ace1baef523d001d5e7eabac9fa466a9159a30bb925cc1327c2d6c4efb89dcaf54e176d1752
|
|
reconsiderable fee failures and skipped transactions
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 https://github.com/bitcoin/bitcoin/pull/28785/commits/1147e00e59e47f27024ec96629993c66a3ce4ef0
achow101:
ACK 1147e00e59e47f27024ec96629993c66a3ce4ef0
murchandamus:
reACK 1147e00e59e47f27024ec96629993c66a3ce4ef0
ismaelsadeeq:
ACK 1147e00e59e47f27024ec96629993c66a3ce4ef0
Tree-SHA512: ac1cd73c2b487a1b99d329875d39d8107c91345a5b0b241d54a6a4de67faf11be69a2721cc732c503024a9cca381dac33d61e187957279e3c82653bea118ba91
|
|
Also, fix iwyu
|
|
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
|
|
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.
|
|
As found by lint-spelling.py using codespell 2.2.6.
|
|
|
|
|
|
|
|
`send(2)` can be interrupted or for another reason it may not fully
complete sending all the bytes. We should be ready to retry the send
with the remaining bytes. This is what `Sock::SendComplete()` does,
thus use it in `Socks5()`.
Since `Sock::SendComplete()` takes a `CThreadInterrupt` argument,
change also the recv part of `Socks5()` to use `CThreadInterrupt`
instead of a boolean.
Easier reviewed with `git show -b` (ignore white-space changes).
|