0420f99f42 Create net_peer_connection unit tests (Jon Atack)
4b834f6499 Allow unit tests to access additional CConnman members (Jon Atack)
34b9ef443b net/rpc: Makes CConnman::GetAddedNodeInfo able to return only non-connected address on request (Sergi Delgado Segura)
94e8882d82 rpc: Prevents adding the same ip more than once when formatted differently (Sergi Delgado Segura)
2574b7e177 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 0420f99f42
kashifs:
> > tACK [0420f9](0420f99f42)
sr-gi:
> > > tACK [0420f9](0420f99f42)
mzumsande:
Tested ACK 0420f99f42
Tree-SHA512: a3a10e748c12d98d439dfb193c75bc8d9486717cda5f41560f5c0ace1baef523d001d5e7eabac9fa466a9159a30bb925cc1327c2d6c4efb89dcaf54e176d1752
1147e00e59 [validation] change package-fee-too-low, return wtxid(s) and effective feerate (glozow)
10dd9f2441 [test] use CheckPackageMempoolAcceptResult in previous tests (glozow)
3979f1afcb [validation] add TxValidationResult::TX_RECONSIDERABLE, TX_UNKNOWN (glozow)
5c786a026a [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 1147e00e59
murchandamus:
reACK 1147e00e59
ismaelsadeeq:
ACK 1147e00e59
Tree-SHA512: ac1cd73c2b487a1b99d329875d39d8107c91345a5b0b241d54a6a4de67faf11be69a2721cc732c503024a9cca381dac33d61e187957279e3c82653bea118ba91
af0fca530e netbase: use reliable send() during SOCKS5 handshake (Vasil Dimov)
1b19d1117c 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 af0fca530e
jonatack:
ACK af0fca530e
pinheadmz:
ACK af0fca530e
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.
`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).
99990194ce Remove WithParams serialization helper (MarcoFalke)
ffffb4af83 scripted-diff: Use ser params operator (MarcoFalke)
fae9054793 test: Use SER_PARAMS_OPFUNC in serialize_tests.cpp (MarcoFalke)
Pull request description:
Every serialization parameter struct already has the `SER_PARAMS_OPFUNC`, except for one in the tests.
For consistency, and to remove verbose code, convert the test to `SER_PARAMS_OPFUNC`, and use it everywhere, then remove the `WithParams` helper.
ACKs for top commit:
ajtowns:
reACK 99990194ce
TheCharlatan:
Re-ACK 99990194ce
Tree-SHA512: be9cae4225a502486fe8d552aaf4b2cd2904a9f73cce9d931c6b7c757594ff1982fcc2c30d00d012cd12b0a9531fd609f8bcd7c94b811e965ac087eb8a3589d3
`CConnman::GetAddedNodeInfo` is used both to get a list of addresses to manually connect to
in `CConnman::ThreadOpenAddedConnections`, and to report about manually added connections in
`getaddednodeinfo`. In both cases, all addresses added to `m_added_nodes` are returned, however
the nodes we are already connected to are only relevant to the latter, in the former they are
actively discarded.
Parametrizes `CConnman::GetAddedNodeInfo` so we can ask for only addresses we are not connected to,
to avoid passing useless information around.
fb3e812277 p2p: return `CSubNet` in `LookupSubNet` (brunoerg)
Pull request description:
Analyzing the usage of `LookupSubNet`, noticed that most cases uses check if the subnet is valid by calling `subnet.IsValid()`, and the boolean returned by `LookupSubNet` hasn't been used so much, see:
29d540b7ad/src/httpserver.cpp (L172-L174)29d540b7ad/src/net_permissions.cpp (L114-L116)
It makes sense to return `CSubNet` instead of `bool`.
ACKs for top commit:
achow101:
ACK fb3e812277
vasild:
ACK fb3e812277
theStack:
Code-review ACK fb3e812277
stickies-v:
Concept ACK, but Approach ~0 (for now). Reviewed the code (fb3e812277) and it all looks good to me.
Tree-SHA512: ba50d6bd5d58dfdbe1ce1faebd80dd8cf8c92ac53ef33519860b83399afffab482d5658cb6921b849d7a3df6d5cea911412850e08f3f4e27f7af510fbde4b254
940a49978c Use type-safe txid types in orphanage (dergoegge)
ed70e65016 Introduce types for txids & wtxids (dergoegge)
cdb14d79e8 [net processing] Use HasWitness over comparing (w)txids (dergoegge)
Pull request description:
We currently have two different identifiers for transactions: `txid` (refering to the hash of a transaction without witness data) and `wtxid` (referring to the hash of a transaction including witness data). Both are typed as `uint256` which could lead to type-safety bugs in which one transaction identifier type is passed where the other would be expected.
This PR introduces explicit `Txid` and `Wtxid` types that (if used) would cause compilation errors for such type confusion bugs.
(Only the orphanage is converted to use these types in this PR)
ACKs for top commit:
achow101:
ACK 940a49978c
stickies-v:
ACK 940a49978c
hebasto:
ACK 940a49978c, I have reviewed the code and it looks OK.
instagibbs:
re-ACK 940a49978c
BrandonOdiwuor:
re-ACK 940a49978c
glozow:
reACK 940a49978c
Tree-SHA512: 55298d1c2bb82b7a6995e96e554571c22eaf4a89fb2a4d7a236d70e0f625e8cca62ff2490e1c179c47bd93153fe6527b56870198f026f5ee7753d64d7a424c92
dd4dcbd4cd [fuzz] Delete i2p target (dergoegge)
Pull request description:
closes #28665
The target is buggy and doesn't reach basic coverage.
ACKs for top commit:
maflcko:
lgtm ACK dd4dcbd4cd
glozow:
ACK dd4dcbd4cd, agree it's better to delete this test until somebody wants to write a better one
Tree-SHA512: b6ca6cad1773b1ceb6e5ac0fd501ea615f66507ef811745799deaaa4460f1700d96ae03cf55b740a96ed8cd2283b3d6738cd580ba97f2af619197d6c4414ca21
0e6f6ebc06 net: remove unused CConnman::FindNode(const CSubNet&) (Vasil Dimov)
9482cb780f netbase: possibly change the result of LookupSubNet() to CJDNS (Vasil Dimov)
53afa68026 net: move MaybeFlipIPv6toCJDNS() from net to netbase (Vasil Dimov)
6e308651c4 net: move IsReachable() code to netbase and encapsulate it (Vasil Dimov)
c42ded3d9b fuzz: ConsumeNetAddr(): avoid IPv6 addresses that look like CJDNS (Vasil Dimov)
64d6f77907 net: put CJDNS prefix byte in a constant (Vasil Dimov)
Pull request description:
`LookupSubNet()` would treat addresses that start with `fc` as IPv6 even if `-cjdnsreachable` is set. This creates the following problems where it is called:
* `NetWhitelistPermissions::TryParse()`: otherwise `-whitelist=` fails to white list CJDNS addresses: when a CJDNS peer connects to us, it will be matched against IPv6 `fc...` subnet and the match will never succeed.
* `BanMapFromJson()`: CJDNS bans are stored as just IPv6 addresses in `banlist.json`. Upon reading from disk they have to be converted back to CJDNS, otherwise, after restart, a ban entry like (`fc00::1`, IPv6) would not match a peer (`fc00::1`, CJDNS).
* `RPCConsole::unbanSelectedNode()`: in the GUI the ban entries go through `CSubNet::ToString()` and back via `LookupSubNet()`. Then it must match whatever is stored in `BanMan`, otherwise it is impossible to unban via the GUI.
These were uncovered by https://github.com/bitcoin/bitcoin/pull/26859.
Thus, flip the result of `LookupSubNet()` to CJDNS if the network base address starts with `fc` and `-cjdnsreachable` is set. Since subnetting/masking does not make sense for CJDNS (the address is "random" bytes, like Tor and I2P, there is no hierarchy) treat `fc.../mask` as an invalid `CSubNet`.
To achieve that, `MaybeFlipIPv6toCJDNS()` has to be moved from `net` to `netbase` and thus also `IsReachable()`. In the process of moving `IsReachable()`, `SetReachable()` and `vfLimited[]` encapsulate those in a class.
ACKs for top commit:
jonatack:
Code review ACK 0e6f6ebc06
achow101:
ACK 0e6f6ebc06
mzumsande:
re-ACK 0e6f6ebc06
Tree-SHA512: 4767a60dc882916de4c8b110ce8de208ff3f58daaa0b560e6547d72e604d07c4157e72cf98b237228310fc05c0a3922f446674492e2ba02e990a272d288bd566
b22810887b miniscript: make GetWitnessSize accurate for tapscript (Pieter Wuille)
8be9851408 test: add tests for miniscript GetWitnessSize (Pieter Wuille)
7ed2b2d430 test: remove mutable global contexts in miniscript fuzzer/test (Pieter Wuille)
Pull request description:
So far, the same algorithm is used to compute an (upper bound on) the maximum witness size for both P2WSH and P2TR miniscript. That's unfortunate, because it means fee estimations for P2TR miniscript will miss out on the generic savings brought by P2TR witnesses (smaller signatures and public keys, specifically).
Fix this by making the algorithm use script context specification calculations, and add tests for it. Also included is a cleanup for the tests to avoid mutable globals, as I found it hard to reason about what exactly was being tested.
ACKs for top commit:
achow101:
ACK b22810887b
darosior:
ACK b22810887b
Tree-SHA512: e4bda7376628f3e91cfc74917cefc554ca16eb5f2a0e1adddc33eb8717c4aaa071e56a40f85a2041ae74ec445a7bd0129bba48994c203e0e6e4d25af65954d9e
fa05a726c2 tidy: modernize-use-emplace (MarcoFalke)
Pull request description:
Constructing a temporary unnamed object only to copy or move it into a container seems both verbose in code and a strict performance penalty.
Fix both issues via the `modernize-use-emplace` tidy check.
ACKs for top commit:
Sjors:
re-utACK fa05a726c2
hebasto:
ACK fa05a726c2.
TheCharlatan:
ACK fa05a726c2
Tree-SHA512: 4408a094f406e7bf6c1468c2b0798f68f4d952a1253cf5b20bdc648ad7eea4a2c070051fed46d66fd37bce2ce6f85962484a1d32826b7ab8c9baba431eaa2765
In order to exacerbate a mistake in the stack size tracking logic,
sometimes pad the witness to make the script execute at the brink of the
stack size limit. This way if the stack size is underestimated for a
script it would immediately fail `VerifyScript`.
We introduce another global that dictates the script context under which
to operate when running the target.
For miniscript_script, just consume another byte to set the context.
This should only affect existing seeds to the extent they contain a
CHECKMULTISIG. However it would not invalidate them entirely as they may
contain a NUMEQUAL or a CHECKSIGADD, and this still exercises a bit of
the parser.
For miniscript_string, reduce the string size by one byte and use the
last byte to determine the context. This is the change that i think
would invalidate the lowest number of existing seeds.
For miniscript_stable, we don't want to invalidate any seed. Instead of
creating a new miniscript_stable_tapscript, simply run the target once
for P2WSH and once for Tapscript (with the same seed).
For miniscript_smart, consume one byte before generating a pseudo-random
node to set the context. We have less regard for seed stability for this
target anyways.
In Tapscript MINIMALIF is a consensus rule, so we can rely on the fact
that the `DUP IF [X] ENDIF` will always put an exact 1 on the stack upon
satisfaction.
It is the equivalent of multi() but for Tapscript, using CHECKSIGADD
instead of CHECKMULTISIG.
It shares the same properties as multi() but for 'n', since a threshold
multi_a() may have an empty vector as the top element of its
satisfaction. It could also have the 'o' property when it only has a
single key, but in this case a 'pk()' is always preferable anyways.
We are going to introduce Tapscript support in Miniscript, for which
some of Miniscript rules and properties change (new or modified
fragments, different typing rules, different resources consumption, ..).
352d5eb2a9 test: getrawaddrman RPC (0xb10c)
da384a286b rpc: getrawaddrman for addrman entries (0xb10c)
Pull request description:
Inspired by `getaddrmaninfo` (#27511), this adds a hidden/test-only `getrawaddrman` RPC. The RPC returns information on all addresses in the address manager new and tried tables. Addrman table contents can be used in tests and during development.
The RPC result encodes the `bucket` and `position`, the internal location of addresses in the tables, in the address object's string key. This allows users to choose to consume or to ignore the location information. If the internals of the address manager implementation change, the location encoding might change too.
```
getrawaddrman
EXPERIMENTAL warning: this call may be changed in future releases.
Returns information on all address manager entries for the new and tried tables.
Result:
{ (json object)
"table" : { (json object) buckets with addresses in the address manager table ( new, tried )
"bucket/position" : { (json object) the location in the address manager table (<bucket>/<position>)
"address" : "str", (string) The address of the node
"port" : n, (numeric) The port number of the node
"network" : "str", (string) The network (ipv4, ipv6, onion, i2p, cjdns) of the address
"services" : n, (numeric) The services offered by the node
"time" : xxx, (numeric) The UNIX epoch time when the node was last seen
"source" : "str", (string) The address that relayed the address to us
"source_network" : "str" (string) The network (ipv4, ipv6, onion, i2p, cjdns) of the source address
},
...
},
...
}
Examples:
> bitcoin-cli getrawaddrman
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getrawaddrman", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
```
ACKs for top commit:
willcl-ark:
reACK 352d5eb2a9
amitiuttarwar:
reACK 352d5eb2a9
stratospher:
reACK 352d5eb.
achow101:
ACK 352d5eb2a9
Tree-SHA512: cc462666b5c709617c66b0e3e9a17c4c81e9e295f91bdd9572492d1cb6466fc9b6d48ee805ebe82f9f16010798370effe5c8f4db15065b8c7c0d8637675d615e
7df4508369 test: improve sock_tests/move_assignment (Vasil Dimov)
5086a99b84 net: remove Sock default constructor, it's not necessary (Vasil Dimov)
7829272f78 net: remove now unnecessary Sock::Get() (Vasil Dimov)
944b21b70a net: don't check if the socket is valid in ConnectSocketDirectly() (Vasil Dimov)
aeac68d036 net: don't check if the socket is valid in GetBindAddress() (Vasil Dimov)
5ac1a51ee5 i2p: avoid using Sock::Get() for checking for a valid socket (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Peeking at the underlying socket file descriptor of `Sock` and checkig if it is `INVALID_SOCKET` is bad encapsulation and stands in the way of testing/mocking/fuzzing.
Instead use an empty `unique_ptr` to denote that there is no valid socket where appropriate or outright remove such checks where they are not necessary.
The default constructor `Sock::Sock()` is unnecessary now after recent changes, thus remove it.
ACKs for top commit:
ajtowns:
ACK 7df4508369
jonatack:
ACK 7df4508369
Tree-SHA512: 9742aeeeabe8690530bf74caa6ba296787028c52f4a3342afd193b05dbbb1f6645935c33ba0a5230199a09af01c666bd3c7fb16b48692a0d185356ea59a8ddbf
edbed31066 chainparams: add signet assumeutxo param at height 160_000 (Sjors Provoost)
b8cafe3871 chainparams: add testnet assumeutxo param at height 2_500_000 (Sjors Provoost)
99839bbfa7 doc: add note about confusing HaveTxsDownloaded name (James O'Beirne)
7ee46a755f contrib: add script to demo/test assumeutxo (James O'Beirne)
42cae39356 test: add feature_assumeutxo functional test (James O'Beirne)
0f64bac603 rpc: add getchainstates (James O'Beirne)
bb05857794 refuse to activate a UTXO snapshot if mempool not empty (James O'Beirne)
ce585a9a15 rpc: add loadtxoutset (James O'Beirne)
62ac519e71 validation: do not activate snapshot if behind active chain (James O'Beirne)
9511fb3616 validation: assumeutxo: swap m_mempool on snapshot activation (James O'Beirne)
7fcd21544a blockstorage: segment normal/assumedvalid blockfiles (James O'Beirne)
4c3b8ca35c validation: populate nChainTx value for assumedvalid chainstates (James O'Beirne)
49ef778158 test: adjust chainstate tests to use recognized snapshot base (James O'Beirne)
1019c39982 validation: pruning for multiple chainstates (James O'Beirne)
373cf91531 validation: indexing changes for assumeutxo (James O'Beirne)
1fffdd76a1 net_processing: validationinterface: ignore some events for bg chain (James O'Beirne)
fbe0a7d7ca wallet: validationinterface: only handle active chain notifications (James O'Beirne)
f073917a9e validationinterface: only send zmq notifications for active (James O'Beirne)
4d8f4dcb45 validation: pass ChainstateRole for validationinterface calls (James O'Beirne)
1e59acdf17 validation: only call UpdatedBlockTip for active chainstate (James O'Beirne)
c6af23c517 validation: add ChainstateRole (James O'Beirne)
9f2318c76c validation: MaybeRebalanceCaches when chain leaves IBD (James O'Beirne)
434495a8c1 chainparams: add blockhash to AssumeutxoData (James O'Beirne)
c711ca186f assumeutxo: remove snapshot during -reindex{-chainstate} (James O'Beirne)
c93ef43e4f bugfix: correct is_snapshot_cs in VerifyDB (James O'Beirne)
b73d3bbd23 net_processing: Request assumeutxo background chain blocks (Suhas Daftuar)
Pull request description:
- Background and FAQ: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
- Prior progress/project: https://github.com/bitcoin/bitcoin/projects/11
- Replaces https://github.com/bitcoin/bitcoin/pull/15606, which was closed due to Github slowness. Original description and commentary can be found there.
---
This changeset finishes the first phase of the assumeutxo project. It makes UTXO snapshots loadable via RPC (`loadtxoutset`) and adds `assumeutxo` parameters to chainparams. It contains all the remaining changes necessary to both use an assumedvalid snapshot chainstate and do a full validation sync in the background.
This may look like a lot to review, but note that
- ~200 lines are a (non-essential) demo shell script
- Many lines are functional test, documentation, and relatively dilute RPC code.
So it shouldn't be as burdensome to review as the linecount might suggest.
- **P2P**: minor changes are made to `init.cpp` and `net_processing.cpp` to make simultaneous IBD across multiple chainstates work.
- **Pruning**: implement correct pruning behavior when using a background chainstate
- **Blockfile separation**: to prevent "fragmentation" in blockfile storage, have background chainstates use separate blockfiles from active snapshot chainstates to avoid interleaving heights and impairing pruning.
- **Indexing**: some `CValidationInterface` events are given with an additional parameter, ChainstateRole, and all indexers ignore events from ChainstateRole::ASSUMEDVALID so that indexation only happens sequentially.
- Have `-reindex` properly wipe snapshot chainstates.
- **RPC**: introduce RPC commands `loadtxoutset` and (hidden) `getchainstates`.
- **Release docs & first assumeutxo commitment**: add notes and a particular assumeutxo hash value for first AU-enabled release.
- This will complete the project and allow use of UTXO snapshots for faster node bootstrap.
The next phase, if it were to be pursued, would be coming up with a way to distribute the UTXO snapshots over the P2P network.
---
### UTXO snapshots
Create your own with `./contrib/devtools/utxo_snapshot.sh`, e.g.
```shell
./contrib/devtools/utxo_snapshot.sh 788000 utxo.dat ./src/bitcoin-cli -datadir=$(pwd)/testdata`)
```
or use the pre-generated ones listed below.
- Testnet: **2'500'000** (Sjors):
- torrent: `magnet:?xt=urn:btih:511e09f4bf853aefab00de5c070b1e031f0ecbe9&dn=utxo-testnet-2500000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969`
- sha256: `79db4b025448cc0ac388d8589a28eab02de53055d181e34eb47391717aa16388`
- Signet: **160'000** (Sjors):
- torrent: `magnet:?xt=urn:btih:9da986cb27b3980ea7fd06b21e199b148d486880&dn=utxo-signet-160000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969`
- sha256: `eeeca845385ba91e84ef58c09d38f98f246a24feadaad57fe1e5874f3f92ef8c`
- Mainnet: **800'000** (Sjors):
- Note: this needs the following commit cherry-picked in: 24deb2022b
- torrent: `magnet:?xt=urn:btih:50ee955bef37f5ec3e5b0df4cf0288af3d715a2e&dn=utxo-800000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969`
### Testing
#### For fun (~5min)
If you want to do a quick test, you can run `./contrib/devtools/test_utxo_snapshots.sh` and follow the instructions. This is mostly obviated by the functional tests, though.
#### For real (longer)
If you'd like to experience a real usage of assumeutxo, you can do that too.
I've cut a new snapshot at height 788'000 (http://img.jameso.be/utxo-788000.dat - but you can do it yourself with `./contrib/devtools/utxo_snapshot.sh` if you want). Download that, and then create a datadir for testing:
```sh
$ cd ~/src/bitcoin # or whatever
# get the snapshot
$ curl http://img.jameso.be/utxo-788000.dat > utxo-788000.dat
# you'll want to do this if you like copy/pasting
$ export AU_DATADIR=/home/${USER}/au-test # or wherever
$ mkdir ${AU_DATADIR}
$ vim ${AU_DATADIR}/bitcoin.conf
dbcache=8000 # or, you know, something high
blockfilterindex=1
coinstatsindex=1
prune=3000
logthreadnames=1
```
Obtain this branch, build it, and then start bitcoind:
```sh
$ git remote add jamesob https://github.com/jamesob/bitcoin
$ git fetch jamesob assumeutxo
$ git checkout jamesob/assumeutxo
$ ./configure $conf_args && make # (whatever you like to do here)
# start 'er up and watch the logs
$ ./src/bitcoind -datadir=${AU_DATADIR}
```
Then, in some other window, load the snapshot
```sh
$ ./src/bitcoin-cli -datadir=${AU_DATADIR} loadtxoutset $(pwd)/utxo-788000.dat
```
You'll see some log messages about headers retrieval and waiting to see the snapshot in the headers chain. Once you get the full headers chain, you'll spend a decent amount of time (~10min) loading the snapshot, checking it, and flushing it to disk. After all that happens, you should be syncing to tip in pretty short order, and you'll see the occasional `[background validation]` log message go by.
In yet another window, you can check out chainstate status with
```sh
$ ./src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
```
as well as usual favorites like `getblockchaininfo`.
ACKs for top commit:
achow101:
ACK edbed31066
Tree-SHA512: 6086fb9a38dc7df85fedc76b30084dd8154617a2a91e89a84fb41326d34ef8e7d7ea593107afba01369093bf8cc91770621d98f0ea42a5b3b99db868d2f14dc2
Exposing address manager table entries in a hidden RPC allows to introspect
addrman tables in tests and during development.
As response JSON object the following FORMAT1 is choosen:
{
"table": {
"<bucket>/<position>": { "address": "..", "port": .., ... },
"<bucket>/<position>": { "address": "..", "port": .., ... },
"<bucket>/<position>": { "address": "..", "port": .., ... },
...
}
}
An alternative would be FORMAT2
{
"table": {
"bucket": {
"position": { "address": "..", "port": .., ... },
"position": { "address": "..", "port": .., ... },
..
},
"bucket": {
"position": { "address": "..", "port": .., ... },
..
},
}
}
FORMAT1 and FORMAT2 have different encodings for the location of the
address in the address manager. While FORMAT2 might be easier to process
for downstream tools, it also mimics internal addrman mappings, which
might change at some point. Users not interested in the address location
can ignore the location key. They don't have to adapt to a new RPC
response format, when the internal addrman layout changes. Additionally,
FORMAT1 is also slightly easier to to iterate in downstream tools. The
RPC response-building implemenation complexcity is lower with FORMAT1
as we can more easily build a "<bucket>/<position>" key than a multiple
"bucket" objects with multiple "position" objects (FORMAT2).
fac29a0ab1 Remove SER_GETHASH, hard-code client version in CKeyPool serialize (MarcoFalke)
fa72f09d6f Remove CHashWriter type (MarcoFalke)
fa4a9c0f43 Remove unused GetType() from OverrideStream, CVectorWriter, SpanReader (MarcoFalke)
Pull request description:
Removes a bunch of redundant, dead or duplicate code.
Uses the idea from and finishes the idea https://github.com/bitcoin/bitcoin/pull/28428 by theuni
ACKs for top commit:
ajtowns:
ACK fac29a0ab1
kevkevinpal:
added one nit but otherwise ACK [fac29a0](fac29a0ab1)
Tree-SHA512: cc805e2f38e73869a6691fdb5da09fa48524506b87fc93f05d32c336ad3033425a2d7608e317decd3141fde3f084403b8de280396c0c39132336fe0f7510af9e
