Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2023 06 tx compression rebase #9

Closed
wants to merge 729 commits into from
Closed

Conversation

TomBriar
Copy link
Owner

No description provided.

fanquake and others added 30 commits February 26, 2024 11:14
b03b206 Fix CI-detected codespell warnings (Lőrinc)

Pull request description:

  Split out the typo fixes encountered in bitcoin#29458 to a separate PR.

ACKs for top commit:
  maflcko:
    ACK b03b206

Tree-SHA512: 99b6fac01ba2ae6e6de9c50d2b481387899844a4b3a77d544c7b8afe7cfd25251a982329688d4739cde8b98ad35afcfd49be7c7cc3dad9bdff1d5915861a206d
0fbf051 depends: fix BDB compilation on OpenBSD (Sebastian Falbesoner)

Pull request description:

  Compiling C++ code with `-D_XOPEN_SOURCE=600` causes problems on OpenBSD. If that define is set, the C++ standard header detection routine in BDB's configure script fails due to a missing type name for `locale_t` (see https://gist.github.com/theStack/b41884e31ebc5cdca3220bcaa674cb70 for the relevant config.log part).

  This results in `HAVE_CXX_STDHEADERS` not being defined, which then it turn leads to the inclusion of `<iostream.h>` (rather than `<iostream>`), which doesn't exist, as described in bitcoin#28963.

  According to a mailing list post discussing a similar problem [1], "OpenBSD provides the POSIX APIs by default", so we don't need this define anyway and can remove it. This fixes the BDB build problem as described in issue bitcoin#28963. See also google/flatbuffers@f87e75a for a similar fix for google's flatbuffer project.

  Tested on OpenBSD 7.4 with clang 13.0.0. Fixes bitcoin#28963.

  [1] https://www.mail-archive.com/tech@openbsd.org/msg63386.html

ACKs for top commit:
  fanquake:
    ACK 0fbf051

Tree-SHA512: 02139e9081ed855e067bfba8c81b54c657417576e553cc1035a916ada9be049358f5e14d756d5f234c5226bd7e943f61c6ae8990c1b152f9125681b7b777c9b3
faeed91 test: Fix intermittent issue in interface_rest.py (MarcoFalke)

Pull request description:

  Fixes:

  ```
   test  2024-02-22T16:15:37.465000Z TestFramework (ERROR): Assertion failed
    Traceback (most recent call last):
    File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
     self.run_test()
    File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/interface_rest.py", line 340, in run_test
     assert_equal(json_obj, mempool_info)
    File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 57, in assert_equal
     raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
   AssertionError: not({'loaded': True, 'size': 3, 'bytes': 312, 'usage': 3600, 'total_fee': Decimal('0.00093600'), 'maxmempool': 300000000, 'mempoolminfee': Decimal('0.00001000'), 'minrelaytxfee': Decimal('0.00001000'), 'incrementalrelayfee': Decimal('0.00001000'), 'unbroadcastcount': 1, 'fullrbf': False} == {'loaded': True, 'size': 3, 'bytes': 312, 'usage': 3600, 'total_fee': Decimal('0.00093600'), 'maxmempool': 300000000, 'mempoolminfee': Decimal('0.00001000'), 'minrelaytxfee': Decimal('0.00001000'), 'incrementalrelayfee': Decimal('0.00001000'), 'unbroadcastcount': 0, 'fullrbf': False})
  ```

  https://cirrus-ci.com/task/4852944378527744?logs=ci#L4436

ACKs for top commit:
  m3dwards:
    ACK bitcoin@faeed91
  mzumsande:
    ACK faeed91

Tree-SHA512: 513422229db45d2586c554b9a466e86848bfcf5280b0f000718cbfc44d93dd1af69e19a56f6ac578f5d7aada74ab0c90d4a9e09a324062b6f9ed239e5e34f540
The MinGW-w64 toolchain links executables to the old msvcrt C Runtime
Library that does not support the `x` modifier for the _wfopen()
function.
The nodes are not connected at this point and no blocks have been mined, so it does not seem do anything
Tested and used all build options on OpenBSD 7.4 with no issues.

Added a note about referring to depends/README.md for detailed instructions on required dependencies.
This was added in reference to a conversation in bitcoin#29443
bit_width is a drop-in replacement with an exact meaning in c++, so there is
no need to continue testing/fuzzing/benchmarking.
…l for MinGW builds

d2fe905 test: Drop `x` modifier in `fsbridge::fopen` call for mingw builds (Hennadii Stepanov)

Pull request description:

  The MinGW-w64 toolchain links executables to the old msvcrt C Runtime Library that does not support the `x` modifier for the [`_wfopen()`](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-wfopen?view=msvc-170) function.

  Fixes bitcoin#29014.

ACKs for top commit:
  maflcko:
    ACK d2fe905
  fanquake:
    ACK d2fe905 - the plan here should still be to migrate to the newer windows runtime.

Tree-SHA512: 0269b66531e58c093ecda3a3e355a20ee8274e165d7e010f8f125881b3c8d4cfe801abdca4605d81efd3b2dbe9a81896968971f6f53da7f6c6093b76b47c5bc9
fa3a410 fuzz: Set -rss_limit_mb=8000 for generate as well (MarcoFalke)
fa4e396 fuzz: Generate with random libFuzzer settings (MarcoFalke)

Pull request description:

  Sometimes a libFuzzer setting like `-use_value_profile=1` helps [0], sometimes it hurts [1].

  [0] bitcoin#20789 (comment)
  [1] bitcoin#27888 (comment)

  By picking a random value, it is ensured that at least some of the runs will have the beneficial configuration set.

  Also, set `-max_total_time` to prevent slow fuzz targets from getting a larger time share, or possibly peg to a single core for a long time and block the python script from exiting for a long time. This can be improved in the future. For example, the python script can exit after some time (bitcoin#20752 (comment)). Alternatively, it can measure if coverage progress was made and run for less time if no progress has been made recently anyway, so that more time can be spent on targets that are new or still make progress.

ACKs for top commit:
  murchandamus:
    utACK fa3a410
  dergoegge:
    utACK fa3a410
  brunoerg:
    light ACK fa3a410

Tree-SHA512: bfd04a76ca09aec612397bae5f3f263a608faa7087697169bd4c506c8195c4d2dd84ddc7fcd3ebbc75771eab618fad840af819114968ca3668fc730092376768
…2transport is enabled

bf5662c test: enable v2 for python p2p depending on global --v2transport flag (Martin Zumsande)
6e9e39d test: Don't use v2transport when it's too slow. (Martin Zumsande)
87549c8 test: enable p2p_invalid_messages.py with v2transport (Martin Zumsande)
5fc9db5 test: enable p2p_sendtxrcncl.py with v2transport (Martin Zumsande)

Pull request description:

  bitcoin#24748 added v2 transport to the python `P2PConnection`, but so far each test that wants to make use of it needs to enable it on an individual basis.
  This PR changes it so that if the test suite is run with `--v2transport` option, v2 is used in each test by default, not only for connections between two bitcoind instances as before, but also wherever `P2PConnection` is used. Individual tests can override this global option.

  To do that, a few tests need to be adjusted.
  In addition, I added a commit to always use v1 in a few select subtests that send a large number of large messages (e.g. large reorgs). These tests don't have a fundamental problem with v2 but become very slow due to the unoptimised python ChaCha20 implementation (~30 minutes on my computer, so probably not suitable to be run in the CI).

  As a result, `python3 test_runner.py --v2transport` should succeed and use `v2` everywhere (unless v1 is chosen explicitly).

  [Edit]: To make the "test each commit" CI pass, several test fixes were squashed into the last commit, which actually enables v2 p2p for `P2PConnection`. I have an unsquashed version at https://github.com/mzumsande/bitcoin/tree/202401_bip324_alltests_unsquashed, in case that helps with review.

ACKs for top commit:
  fjahr:
    tACK bf5662c
  vasild:
    ACK bf5662c
  stratospher:
    reACK bf5662c.
  theStack:
    Tested ACK bf5662c

Tree-SHA512: 4f5a08248ba8a755f7d0f48deb2b79bef03292345cacb7deef01be955481093800e4e56ff218ea56734eef5de1fb3ab0f04657447ea27d393bb536539d7b289d
Now that --enable-suppress-external-warnings is on by default, we can
drop it.
fccfdb2 doc: Update OpenBSD build docs to 7.4 (Jesse Barton)

Pull request description:

  Updated OpenBSD Build doc for 7.4 after testing all build options. No issues on my end.

  Also added a note about referring to depends/README.md for detailed instructions on required dependencies.
  This was added in reference to a conversation in bitcoin#29443

ACKs for top commit:
  fanquake:
    ACK fccfdb2
  theStack:
    lgtm ACK fccfdb2

Tree-SHA512: be6d22b605140b37a71e11c5bbed54f60655832d78cd3cb221eddc77c7621a65c0d71baf436f90819be536d9b5dbf1a0b2c82b6b23d62356addc495403f2ba35
We preemptively perform a block mutation check before further processing
a block message (similar to early sanity checks on other messsage
types). The main reasons for this change are as follows:

- `CBlock::GetHash()` is a foot-gun without a prior mutation check, as
  the hash returned only commits to the header but not to the actual
  transactions (`CBlock::vtx`) contained in the block.
- We have observed attacks that abused mutated blocks in the past, which
  could have been prevented by simply not processing mutated blocks
  (e.g. bitcoin#27608).
Slight performance improvement by avoiding duplicate work.
This code has been dead since bitcoin#28967.

Required as a precondition for replacing Boost.Process with
cpp-subprocess to make diff for this code meaningful and reviewable.

The plan is to reintroduce Windows-specific code in this test
simultaneously with enabling Windows support in cpp-subprocess.
b60d2b7334 Merge bitcoin-core/crc32c-subtree#6: Fix UBSan "misaligned-pointer-use" warning on aarch64
1ac401e32b Fix UBSan "misaligned-pointer-use" warning on aarch64

git-subtree-dir: src/crc32c
git-subtree-split: b60d2b733406cc64025095c6c2cb3933e222b529
…oncept

This removes the only remaining autoconf macro in our serialization code,
so it can now be used trivially and safely out-of-tree.
6fa61e3 doc: Fix Broken Links (Justin Dhillon)

Pull request description:

  ### Summery

  Here is what I have fixed:

  http://voorloopnul.com/blog/a-python-netstat-in-less-than-100-lines-of-code/
   --> https://web.archive.org/web/20190424172231/http://voorloopnul.com/blog/a-python-netstat-in-less-than-100-lines-of-code/

  ### Support my work

  These links were found with [link-inspector](https://github.com/justindhillon/link-inspector). If you find this PR useful, give the repo a ⭐

ACKs for top commit:
  fjahr:
    ACK 6fa61e3

Tree-SHA512: ba83badfc8a89f33813801f749bcd7ad41d4c9c817ece76f3bb1b60f24c28e99cfccc485a0ba059ec2c1134e8ffb5fa37fdc9835e553229ee5b1167c9b2e8d1f
ryanofsky and others added 28 commits March 18, 2024 11:28
Add a test for a CheckBlockIndex crash that would happen before previous
"assumeutxo: Get rid of faked nTx and nChainTx values" commit.

The crash was an assert failure in the (pindex->nChainTx == pindex->nTx +
prev_chain_tx) check that would previously happen if a snapshot was loaded, and
a block was submitted which forked from the chain before the snapshot block and
after the last downloaded background chain block. This block would not be
marked assumed-valid because it would not be an ancestor of the snapshot, and
it would have nTx set, nChainTx unset, and prev->nChainTx set with a fake
value, so the assert would fail. After the fix, prev->nChainTx is unset instead
of being set to a fake value, so the assert succeeds. This test was originally
posted by maflcko in
bitcoin#29261 (comment)

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Add a test for a CheckBlockIndex crash that would happen before previous
"assumeutxo: Get rid of faked nTx and nChainTx values" commit.

The crash was an assert failure in the (pindex->nChainTx == pindex->nTx +
prev_chain_tx) check that would previously happen if the snapshot block was
submitted after loading the snapshot and downloading a few blocks after the
snapshot. In that case ReceivedBlockTransactions() previously would overwrite
the nChainTx value of the submitted snapshot block with a fake value based on
the previous block, so the (pindex->nChainTx == pindex->nTx + prev_chain_tx)
check would later fail on the first block after the snapshot. This test was
originally posted by Martin Zumsande <mzumsande@gmail.com> in
bitcoin#29370 (comment)

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
64722e4 ci: Drop `--enable-c++20` option (Hennadii Stepanov)

Pull request description:

  This option has ceased to exist since bitcoin#28349.

ACKs for top commit:
  maflcko:
    ACK 64722e4

Tree-SHA512: bd392c331f775605615e1b236682269b83a1e6363a4d3f09c4d8d54495cf3d22973a921ebf6b8a9f813ba6c024d3324761f3291aaf7f473995f5eaa4c195bc43
Flag adds complexity and is not currently used for anything.
…bmitpackage

38f70ba RPC: Add maxfeerate and maxburnamount args to submitpackage (Greg Sanders)

Pull request description:

  Resolves bitcoin#28949

  I couldn't manage to do it very cleanly outside of (sub)package evaluation itself, since it would change the current interface very heavily. Instead I threaded through the max fee argument and used that directly via ATMPArgs. From that perspective, this is somewhat a reversion from bitcoin#19339. In a post-cluster mempool world, these checks could be consolidated to right after the given (ancestor) package is linearized/chunked, by just checking the feerate of the top chunk and rejecting the submission entirely if the top chunk is too high.

  The implication here is that subpackages can be submitted to the mempool prior to hitting this new fee-based error condition.

ACKs for top commit:
  ismaelsadeeq:
    Re-ACK bitcoin@38f70ba 👍🏾
  glozow:
    ACK 38f70ba with some non-blocking nits
  murchandamus:
    LGTM, code review ACK 38f70ba

Tree-SHA512: 38212aa9de25730944cee58b0806a3d37097e42719af8dd7de91ce86bb5d9770b6f7c37354bf418bd8ba571c52947da1dcdbb968bf429dd1dbdf8715315af18f
…t test

626f8e3 fuzz: actually test garbage >64b in p2p transport test (Pieter Wuille)

Pull request description:

  This fixes an oversight from bitcoin#28196: in the `p2p_transport_bidirectional_v2` fuzz test, when the desired garbage length is over 64 bytes, the code would actually use garbage length 0. Fix this.

ACKs for top commit:
  instagibbs:
    ACK bitcoin@626f8e3
  brunoerg:
    crACK 626f8e3

Tree-SHA512: f6346367adb10464b6c9d20aef43625531d2a4d8110887ad03214b8c1907b83560f2dd5b5415e2180a40b4cd276d51881b32b60c740471b5c6bb218aa19848d8
432a542 test: fix intermittent failures with test=addrman (Martin Zumsande)

Pull request description:

  The `nKey` of the addrman is generated the first time the node is started with an empty `peers.dat`. Therefore, restarting a node or turning it off and on again won't make a previously non-deterministic addrman deterministic.
  This could lead to intermittent failures in `feature_asmap.py` and `rpc_net.py`

  Fixes bitcoin#29634

ACKs for top commit:
  kevkevinpal:
    ACK [432a542](bitcoin@432a542)
  stratospher:
    Tested ACK 432a542.
  brunoerg:
    crACK 432a542
  0xB10C:
    ACK 432a542

Tree-SHA512: a8e284baeb0be2df7284b8a2792cb9edc9e2d5b877a3b29ab7277ffdb75b17efa58a4d42576441eb493cd518e7c5ffdb05597b27e42b5001cf1a80e78bb04c83
fae70ba ci: Better tidy errors (MarcoFalke)

Pull request description:

  Currently tidy errors are not nice, because the user may have to scroll up to see them in a large block of text. See for example (before) https://github.com/bitcoin/bitcoin/runs/19670551485

  Fix that by `tee`ing the output to a file and summarizing the errors in the end again. See for example (after): https://github.com/bitcoin/bitcoin/runs/22647850662

ACKs for top commit:
  hebasto:
    ACK fae70ba, logs with errors look cleaner.
  TheCharlatan:
    ACK fae70ba

Tree-SHA512: dcaea557fed40089409d16ce2cbaa8a9cfbf047f601d5daadfee0823b0eed7badc12d803addc0b7b6bb3f1eaf5c787fccb2488475d32c4efd80835f386f761dd
6e873df serfloat: improve/simplify tests (Pieter Wuille)
b45f1f5 serfloat: do not test encode(bits)=bits anymore (Pieter Wuille)

Pull request description:

  Closes bitcoin#28941.

  Our current tests for serfloat verify two distinct properties:
  1. Whether they roundtrip `double`->`uint64_t`->`double` (excluding NaN values) on all systems.
  2. Whether on systems with a typical floating point unit that encoding matches the hardware representation, as before v22.0, we would dump the hardware representation directly to disk and we wanted to retain compatibility with that.

  bitcoin#28941 seems to show that the second property doesn't always hold, but just for "subnormal" numbers (below $2^{-1021}$). Since we don't care about encoding these numbers, we could exclude such subnormal numbers from the hardware-identical representation test, but this PR goes further and just drops the second property entirely, as I don't think we care about edge-case compatibility with pre-v22.0 code for fee_estimates.dat (the only place it is used).

ACKs for top commit:
  glozow:
    ACK 6e873df
  fanquake:
    ACK 6e873df - It's not as much of a priority, but I think we could still backport this.

Tree-SHA512: e18ceee0753a7ee7e999fdfa10b014dc5bb67b6ef79522a0f8c76b889adcfa785772fc26ed7559bcb5a09a9938e243bb54eedd9549bc59080a2c8090155e2267
…loop

This avoids having commit print a needless error message during init.

Co-authored-by: furszy <mfurszy@protonmail.com>
…able service flags

2f23987 test: p2p: check limited peers desirability (depending on best block depth) (Sebastian Falbesoner)
c4a67d3 test: p2p: check disconnect due to lack of desirable service flags (Sebastian Falbesoner)
405ac81 test: p2p: support disconnect waiting for `add_outbound_p2p_connection` (Sebastian Falbesoner)

Pull request description:

  This PR adds missing test coverage for disconnecting peers which don't offer the desirable service flags in their VERSION message:
  https://github.com/bitcoin/bitcoin/blob/5f3a0574c45477288bc678b15f24940486084576/src/net_processing.cpp#L3384-L3389
  This check is relevant for the connection types "outbound-full-relay", "block-relay-only" and "addr-fetch" (see `CNode::ExpectServicesFromConn(...)`). Feeler connections always disconnect, which is also tested here.

  In lack of finding a proper file where this test would fit in, I created a new one. Happy to take suggestions there.

ACKs for top commit:
  davidgumberg:
    reACK bitcoin@2f23987
  itornaza:
    tested ACK 2f23987
  fjahr:
    re-utACK 2f23987
  cbergqvist:
    re ACK 2f23987
  stratospher:
    tested ACK 2f23987. 🚀

Tree-SHA512: cf75d9d4379d0f34fa1e13152e6a8d93cd51b9573466ab3a2fec32dc3e1ac49b174bd1063cae558bc736b111c8a6e7058b1b57a496df56255221bf367d29eb5d
faecf3a ci: Bump msan to llvm-18 (MarcoFalke)

Pull request description:

  Last one: bitcoin#28476

ACKs for top commit:
  fanquake:
    ACK faecf3a - There is now a 18.1.2, but given it doesn't fix the instrumenting in libunwind, we don't need that here. I've tested that both jobs are now working on both arches.

Tree-SHA512: 489c0b343bdc732687131317a570f3efbb18a3f548736d739da90d1a1e784df1dbb208c2da8a2a7740f27f961a841c477487a14c4d59910368f651225f0779b2
…ations by caching last header

99afb9d refactor: init, simplify index shutdown code (furszy)
0faafb5 index: decrease ThreadSync cs_main contention (furszy)
f1469eb index: cache last block filter header (furszy)
a6756ec index: blockfilter, decouple header lookup into its own function (furszy)
331f044 index: blockfilter, decouple Write into its own function (furszy)
bcbd7eb bench: basic block filter index initial sync (furszy)

Pull request description:

  Work decoupled from bitcoin#26966 per request.

  The aim is to remove an unnecessary disk read operation that currently takes place with every new arriving block (or scanned block during background sync). Instead of reading the last filter header from disk merely to access its hash for constructing the next filter, this work caches it, occupying just 32 more bytes in memory.

  Also, reduces `cs_main` lock contention during the index initial sync process. And, simplifies the indexes initialization and shutdown procedure.

  Testing Note:
  To compare the changes, added a pretty basic benchmark in the second commit. Alternatively, could also test the changes by timing the block filter sync from scratch on any network; start the node with `-blockfilterindex` and monitor the logs until the syncing process finish.

  Local Benchmark Results:

  *Master (c252a0f):
  |               ns/op |                op/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |      132,042,516.60 |                7.57 |    0.3% |      7.79 | `BlockFilterIndexSync`

  *PR (43a212c):
  |               ns/op |                op/s |    err% |     total | benchmark
  |--------------------:|--------------------:|--------:|----------:|:----------
  |      126,915,841.60 |                7.88 |    0.6% |      7.51 | `BlockFilterIndexSync`

ACKs for top commit:
  Sjors:
    re-ACK 99afb9d
  achow101:
    ACK 99afb9d
  TheCharlatan:
    Re-ACK 99afb9d
  andrewtoth:
    ACK 99afb9d

Tree-SHA512: 927daadd68f4ee1ca781a89519539b895f5185a76ebaf525fbc246ea8dcf40d44a82def00ac34b188640802844b312270067f1b33e65a2479e06be9169c616de
…s already there

dfcef53 blockstorage: do not flush block to disk if it is already there (Matthew Zipkin)

Pull request description:

  Closes bitcoin#2039

  When reindexing from flat-file block storage there is no need to write anything back to disk, since the block data is already there. This PR skips flushing to disk those blocks that already have a known position in the datastore. Skipping this means that users can write-protect the `blk` files on disk which may be useful for security or even safely sharing that data between multiple bitcoind instances.

  `FindBlockPos()` may also flush the undo data file, but again this is skipped if the corresponding block position is known, like during the initial stage of a reindex when block data is being indexed. Once the block index is complete the validation mechanism will call `ConnectBlock()` which will save undo data at that time.

  The call stack looks like this:

  ```
  init()
  ThreadImport() <-- process fReindex flag
  LoadExternalBlockFile()
  AcceptBlock()
  SaveBlockToDisk()
  FindBlockPos()
  FlushBlockFile() <-- unnecessary if block is already on disk
  ```

  A larger refactor of this part of the code was started by mzumsande here:  https://github.com/mzumsande/bitcoin/tree/202207_refactor_findblockpos including this fix, reviewers can let me know if the changes should be combined.

ACKs for top commit:
  sipa:
    utACK dfcef53
  mzumsande:
    re-ACK dfcef53
  achow101:
    ACK dfcef53
  furszy:
    Rebase diff ACK dfcef53.

Tree-SHA512: 385c5ac1288b325135398d0ddd3ab788fa98cc0ca19bd2474c74039f2ce70d5088c1d1c9d4dd10aefcbd4c757767ec5805d07ba8cee9289a66f96e6f9eaa5279
…lues

9d9a745 assumeutxo: Remove BLOCK_ASSUMED_VALID flag (Ryan Ofsky)
ef174e9 test: assumeutxo snapshot block CheckBlockIndex crash test (Ryan Ofsky)
0391458 test: assumeutxo stale block CheckBlockIndex crash test (Ryan Ofsky)
ef29c8b assumeutxo: Get rid of faked nTx and nChainTx values (Ryan Ofsky)
9b97d5b doc: Improve comments describing setBlockIndexCandidates checks (Ryan Ofsky)
0fd915e validation: Check GuessVerificationProgress is not called with disconnected block (Ryan Ofsky)
63e8fc9 ci: add getchaintxstats ubsan suppressions (Ryan Ofsky)
f252e68 assumeutxo test: Add RPC test for fake nTx and nChainTx values (Ryan Ofsky)

Pull request description:

  The `PopulateAndValidateSnapshot` function introduced in f6e2da5 from bitcoin#19806 has been setting fake `nTx` and `nChainTx` values that can show up in RPC results (bitcoin#29328) and make `CBlockIndex` state hard to reason about, because it is difficult to know whether the values are real or fake.

  Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the values are unknown, instead of faking them. Also drop no-longer needed `BLOCK_ASSUMED_VALID` flag.

  Dropping the faked values also fixes assert failures in the `CheckBlockIndex` `(pindex->nChainTx == pindex->nTx + prev_chain_tx)` check that could happen previously if forked or out-of-order blocks before the snapshot got submitted while the snapshot was being validated. The PR includes two commits adding tests for these failures and describing them in detail.

  Compatibility note: This change could cause new `-checkblockindex` failures if a snapshot was loaded by a previous version of Bitcoin Core and not fully validated, because fake `nTx` values will have been saved to the block index. It would be pretty easy to avoid these failures by adding some compatibility code to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake (when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a little simpler not to worry about being compatible in this case.

ACKs for top commit:
  Sjors:
    re-ACK 9d9a745
  achow101:
    ACK 9d9a745
  mzumsande:
    Tested ACK 9d9a745
  maflcko:
    ACK 9d9a745 🎯

Tree-SHA512: b1e1e2731ec36be30d5f5914042517219378fc31486674030c29d9c7488ed83fb60ba7095600f469dc32f0d8ba79c49ff7706303006507654e1762f26ee416e0
…lization

f65b0f6 index: Move last_locator_write_time and logging to end of threadsync loop (Fabian Jahr)

Pull request description:

  In the index sync thread, when initializing an index for the first time, stop callng BaseIndex::Commit when m_best_block_index is null, to avoid a spurious "failed to commit" error from that function. This error started happening in commit bitcoin@7878f97 from bitcoin#25494 and was reported by pstratem in bitcoin#26903 with an alternate fix.

ACKs for top commit:
  achow101:
    ACK f65b0f6
  ryanofsky:
    Code review ACK f65b0f6. Just moved log "Syncing" log line since last commit to avoid having to call now() twice.
  furszy:
    ACK f65b0f6
  TheCharlatan:
    ACK f65b0f6

Tree-SHA512: afa8f05786318d36346d167ff53ea0b3bc8abdb0ad04465d199dc3eb91e9f837369e24fcb7e24b5757b02d698ec504e61da6ac365eaf006c874fc07a424a7e20
…725bf189c2b68bd6b5a

cf5faf7 guix: bump time-machine to dc4842797bfdc5f9f3f5f725bf189c2b68bd6b5a (fanquake)

Pull request description:

  This includes a commit to fix building LLVM 17 on riscv64, see https://git.savannah.gnu.org/cgit/guix.git/commit/?id=4e26331a5ee87928a16888c36d51e270f0f10f90.

  Followup to discussion in bitcoin#28880 (comment).

  If you don't have riscv64 hardware, this can be tested with the following:
  ```bash
  # observe failure when cross-compiling using our current time-machine
  guix time-machine --commit=d5ca4d4fd713a9f7e17e074a1e37dda99bbb09fc -- build --target=riscv64-linux-gnu  llvm
  ....
  riscv64-linux-gnu-ld: CMakeFiles/dsymutil.dir/dsymutil.cpp.o: undefined reference to symbol '__atomic_fetch_and_1@@LIBATOMIC_1.0'
  riscv64-linux-gnu-ld: /gnu/store/i4ga0pnr1b74bir2bjyp8mcrrbsvk7d3-gcc-cross-riscv64-linux-gnu-11.3.0-lib/riscv64-linux-gnu/lib/libatomic.so.1:
    error adding symbols: DSO missing from command line
  collect2: error: ld returned 1 exit status

  # build success when using the new time-machine
  guix time-machine --commit=dc4842797bfdc5f9f3f5f725bf189c2b68bd6b5a -- build --target=riscv64-linux-gnu  llvm
  ....
  grafting '/gnu/store/7y0j0y8jaz4mjx2nz0y42wdnxxjp6id6-llvm-17.0.6-opt-viewer' -> '/gnu/store/8xvahrrjscbprh6cjj0qp5bm9mm78wwa-llvm-17.0.6-opt-viewer'...
  grafting '/gnu/store/bjhw648bz7ijd2p9hgzzdbw1q8hpagk8-llvm-17.0.6' -> '/gnu/store/x50qi8i2ywgpx6azv4k55ms0w5xjxxg5-llvm-17.0.6'...
  successfully built /gnu/store/q9xvk8gzzvb4dxfzf6yi5164zd0d1vj2-llvm-17.0.6.drv
  ```

  Also includes at least:
  Linux Headers 6.1.67 -> 6.1.80

ACKs for top commit:
  TheCharlatan:
    ACK cf5faf7
  hebasto:
    ACK cf5faf7, tested on x86_64 hardware as described in the PR description.

Tree-SHA512: b49d4f90effeec666b12b5447a24c90315b82675cfc166bc1230ac173134bab6b277fc7e064bbb75e990275165b2b27d88e4ec1cdeea4750541ec6443cb50f41
…nsaction, For valid assets_tests run compression tests, Added unit tests for new primitives
@TomBriar TomBriar force-pushed the 2023-06--tx-compression-rebase branch from f629554 to 6f92525 Compare March 21, 2024 15:35
@TomBriar TomBriar closed this Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.