fa79a881ce refactor: P2P transport without serialize version and type (MarcoFalke)
fa9b5f4fe3 refactor: NetMsg::Make() without nVersion (MarcoFalke)
66669da4a5 Remove unused Make() overload in netmessagemaker.h (MarcoFalke)
fa0ed07941 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 fa79a881ce
Tree-SHA512: 785b413580d980f51f0d4f70ea5e0a99ce14cd12cb065393de2f5254891be94a14f4266110c8b87bd2dbc37467676655bce13bdb295ab139749fcd8b61bd5110
fa6b053b5c 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 fa6b053b5c
glozow:
reACK fa6b053b5c
ismaelsadeeq:
ACK fa6b053b5c
Tree-SHA512: ded2ce3d81bc944b828263534e3178a1e45a914fe8e024f4a14c6561a73e301820944ecc75dd704b3d4221a7a3a5c0597ccab79546250c1197609ee981fe324e
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
This was only explicitly used in the tests, where it can be replaced by
wrapping the original raw file pointer into a CAutoFile on creation and
then calling CAutoFile::fclose().
Also, it was used in LoadExternalBlockFile(), where it can also be
replaced by the (implicit call to the) CAutoFile destructor after
wrapping the original raw file pointer in a CAutoFile.
While touching all constructors in the previous commit, the class name
can be adjusted to comply with the style guide.
-BEGIN VERIFY SCRIPT-
sed -i 's/CBufferedFile/BufferedFile/g' $( git grep -l CBufferedFile )
-END VERIFY SCRIPT-
New code can call the method without having first to retrieve the raw
FILE* pointer via Get().
Also, move implementation to the cpp file. Can be reviewed with:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
* Add m_ prefix to the std::FILE member variable
* Add std namespace where possible, to avoid confusion with member
functions of the same name.
* Add AutoFile::feof() member function, to be used in place of
std::feof(AutoFile::Get())
* Simplify fclose() in terms of release()
* Fix typo in the error message in the ignore member function.
The shift operators will call the write or read member function, which
already does the check. Also, call sites are free to directly call
::(Un)Serialize(s, obj) to skip this check, so removing it increases
consistency.
DataStream Serialize method has surprising behavior because it just serializes
raw bytes without a length prefix. When you serialize a string or vector, a
length prefix is serialized before the raw object contents so the object can be
unambiguously deserialized later. But DataStreams don't support deserializing
at all and just dump the raw bytes.
Having this inconsistency is not necessary and could be confusing (see
https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212315030) so this
PR just drops the DataStream::Serialize method.
Avoid use of the expensive mod operator (%) when calculating the
buffer offset. No functional difference.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
fa47b28dfc refactor: Remove unused CDataStream SerializeMany constructor (MarcoFalke)
Pull request description:
Seems odd to have an unused method. Moreover, the function is fragile and dangerous, because one could have a `std::vector vec_a` and type `CDataStream{vec_a, 0, 0}.size()` and `CDataStream{0, 0, vec_a}.size()`, assuming they are the same thing, when in fact they are not. (The first takes over the memory as is, the second serializes the vector).
So my suggestion would be to remove the unused method and introduce a new method when this functionality is needed. For example: `static DataStream FromMany(Args&&... args)`.
ACKs for top commit:
stickies-v:
ACK fa47b28dfc
Tree-SHA512: 9593a034b997e33a0794f779f76f02425b1097b218cf8cb1facb7f874fa69da328ce567a79138015baeebe004ae7d103dda4f64f83e8ad375b6dae6b66d3d950
SkipTo() reads data from the file into the CBufferedFile object
(memory), but, unlike this object's read() method, SkipTo() doesn't
transfer data into a caller's memory buffer. This is useful because
after skipping forward in the stream in this way, the user can, if
needed, rewind the stream (SetPos()) and access the object's memory
buffer including ranges that were skipped over (without needing to
read from the disk file).
bf95976061 doc: add note about snapshot chainstate init (James O'Beirne)
e4d7995286 test: add testcases for snapshot initialization (James O'Beirne)
cced4e7336 test: move-only-ish: factor out LoadVerifyActivateChainstate() (James O'Beirne)
51fc9241c0 test: allow on-disk coins and block tree dbs in tests (James O'Beirne)
3c361391b8 test: add reset_chainstate parameter for snapshot unittests (James O'Beirne)
00b357c215 validation: add ResetChainstates() (James O'Beirne)
3a29dfbfb2 move-only: test: make snapshot chainstate setup reusable (James O'Beirne)
8153bd9247 blockmanager: avoid undefined behavior during FlushBlockFile (James O'Beirne)
ad67ff377c validation: remove snapshot datadirs upon validation failure (James O'Beirne)
34d1590331 add utilities for deleting on-disk leveldb data (James O'Beirne)
252abd1e8b init: add utxo snapshot detection (James O'Beirne)
f9f1735f13 validation: rename snapshot chainstate dir (James O'Beirne)
d14bebf100 db: add StoragePath to CDBWrapper/CCoinsViewDB (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: https://github.com/bitcoin/bitcoin/pull/15606)
---
Half of the replacement for #24232. The original PR grew larger than expected throughout the review process.
This change adds the ability to initialize a snapshot-based chainstate during init if one is detected on disk. This is of course unused as of now (aside from in unittests) given that we haven't yet enabled actually loading snapshots.
Don't be scared! There are some big move-only commits in here.
Accompanying changes include:
- moving the snapshot coinsdb directory from being called `chainstate_[base blockhash]` to `chainstate_snapshot`, since we only support one snapshot in use at a time. This simplifies some logic, but it necessitates writing that base blockhash out to a file within the coinsdb dir. See [discussion here](https://github.com/bitcoin/bitcoin/pull/24232#discussion_r832762880).
- adding a simple fix in `FlushBlockFile()` that avoids a crash when attemping to flush to disk before `LoadBlockIndexDB()` is called, which happens when calling `MaybeRebalanceCaches()` during multiple chainstate init.
- improving the unittest to allow testing with on-disk chainstates - necessary to test a simulated restart and re-initialization.
ACKs for top commit:
naumenkogs:
utACK bf95976061
ariard:
Code Review ACK bf9597606
ryanofsky:
Code review ACK bf95976061. Changes since last review: rebasing, switching from CAutoFile to AutoFile, adding comments, switching from BOOST_CHECK to Assert in test util, using chainman.GetMutex() in tests, destroying one ChainstateManager before creating a new one in tests
fjahr:
utACK bf95976061
aureleoules:
ACK bf95976061
Tree-SHA512: 15ae75caf19f8d12a12d2647c52897904d27b265a7af6b4ae7b858592eeadb8f9da6c2394b6baebec90adc28742c053e3eb506119577dae7c1e722ebb3b7bcc0
This changes the snapshot's leveldb chainstate dir name from
`chainstate_[blockhash]` to `chainstate_snapshot`. This simplifies
later logic that loads snapshot data, and enforces the limitation
of a single snapshot at any given time.
Since we still need to persis the blockhash of the base block, we
write that out to a file (`chainstate_snapshot/base_blockhash`) for
later use during initialization, so that we can reinitialize the
snapshot chainstate.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
This removes the ability to set an offset in the SpanReader constructor,
as the current code is broken. All call sites use pos=0, so it is actually
unused. If future call sites need it, SpanReader{a, b, c, d} is equivalent
to SpanReader{a, b, c.subspan(d)}.
It also removes the ability to deserialize from SpanReader directly from
the constructor. This too is unused, and can be more idiomatically
simulated using (SpanReader{a, b, c} >> x >> y >> z) instead of
SpanReader{a, b, c, x, y, z}.
fa29272459 Remove redundant MakeUCharSpan wrappers (MarcoFalke)
faf4aa2f47 Remove CDataStream::Init in favor of C++11 member initialization (MarcoFalke)
fada14b948 Treat CDataStream bytes as uint8_t (MarcoFalke)
fa8bdb048e refactor: Drop CDataStream constructors in favor of one taking a Span of bytes (MarcoFalke)
faa96f841f Remove unused CDataStream methods (MarcoFalke)
Pull request description:
Using `uint8_t` for raw bytes has a style benefit:
* The signedness is clear from reading the code, as it does not depend on the architecture
Other clean-ups in this pull include:
* Remove unused methods
* Constructor is simplified with `Span`
* Remove `Init()` member in favor of C++11 member initialization
ACKs for top commit:
laanwj:
code review ACK fa29272459
theStack:
ACK fa29272459🍾
Tree-SHA512: 931ee28bd99843d7e894b48e90e1187ffb0278677c267044b3c0c255069d9bbd9298ab2e539b1002a30b543d240450eaec718ef4ee95a7fd4be0a295e926343f
