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.

Sign up for GitHub

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

Jump to bottom

backport: merge bitcoin#19806, #21582, #21584, #21270, #21789, #22415, #21525, #21592 (assumeutxo project backports, part 3) #5236

Merged
merged 8 commits into from
Jun 6, 2023
Merged

backport: merge bitcoin#19806, #21582, #21584, #21270, #21789, #22415, #21525, #21592 (assumeutxo project backports, part 3) #5236

merged 8 commits into from
Jun 6, 2023

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Mar 5, 2023 •
edited
Loading

@kwvg kwvg changed the base branch from master to develop March 5, 2023 20:13
@kwvg kwvg changed the title backports: merge bitcoin#19806, #21523, #21582, #21584, partial #17938 (assumeutxo project backports, part 3) backport: merge bitcoin#19806, #21523, #21582, #21584, partial #17938 (assumeutxo project backports, part 3) Mar 5, 2023
@kwvg kwvg force-pushed the utxo_assume branch 12 times, most recently from 7483db9 to 4534f62 Compare March 13, 2023 16:16
@kwvg kwvg force-pushed the utxo_assume branch 2 times, most recently from 34c07a7 to ba8bc46 Compare March 16, 2023 08:55
@github-actions
Copy link

This pull request has conflicts, please rebase.

@kwvg kwvg force-pushed the utxo_assume branch 3 times, most recently from 8dc73c0 to f965d97 Compare March 27, 2023 14:52
@github-actions
Copy link

github-actions bot commented Apr 4, 2023

This pull request has conflicts, please rebase.

@kwvg kwvg force-pushed the utxo_assume branch 2 times, most recently from 798ca0f to ac60187 Compare April 5, 2023 22:34
@kwvg kwvg requested a review from knst June 1, 2023 18:51
@github-actions
Copy link

github-actions bot commented Jun 4, 2023

This pull request has conflicts, please rebase.

@kwvg kwvg requested a review from UdjinM6 June 6, 2023 11:03
UdjinM6
UdjinM6 previously approved these changes Jun 6, 2023
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, utACK

@github-actions
Copy link

github-actions bot commented Jun 6, 2023

This pull request has conflicts, please rebase.

@kwvg kwvg requested a review from UdjinM6 June 6, 2023 17:11
UdjinM6
UdjinM6 approved these changes Jun 6, 2023
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-utACK

PastaPastaPasta
PastaPastaPasta approved these changes Jun 6, 2023
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK for merging via merge commit

rebase looks correct; merging to avoid further conflicts

@PastaPastaPasta PastaPastaPasta merged commit 87b8acf into dashpay:develop Jun 6, 2023
@kwvg kwvg mentioned this pull request Jul 16, 2024
Merged
5 tasks
PastaPastaPasta added a commit that referenced this pull request Jul 19, 2024
@PastaPastaPasta
Merge #6119: fix: move g_txindex initialization out of erroneous lo…
5f9f05e 
…cation and into constructor

77915de test: run `Interrupt()` before `Stop()`, add additional sanity check (Kittywhiskers Van Gogh)
a376e93 fix: init `g_txindex` only once, downgrade from assert to exception (Kittywhiskers Van Gogh)

Pull request description:

  ## Motivation

  `g_txindex` should be initialized in `TestChainSetup`'s constructor but when backporting [bitcoin#19806](6bf39d7#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R289) ([dash#5236](#5236)), portions of the constructor were split into `TestChainSetup::mineBlocks()`, `g_txindex`'s init was left behind in the latter instead of the former.

  This meant that every `mineBlocks()` call would re-create a `TxIndex` instance, which is not intended behaviour; and was recorded to cause `heap-use-after-free`s ([comment](#6085 (comment)), also the reason this PR was opened).

  This PR aims to resolve that.

  ## Additional Information

  * Crashes stemming from previous attempts (except for one attempt) were not reproducible with my regular local setup (`depends` built with Clang 16, Dash Core built with Clang 16, set of debug-oriented flags, unit tests run using `./src/test/test_dash`).
    * Attempting to rebuild Dash Core with GCC 9 was insufficient, required to rebuild depends with GCC 9 as well
    * `configure`'d with `CC=gcc CXX=g++ CPPFLAGS="-DDEBUG_LOCKORDER -DARENA_DEBUG" ./configure --prefix=$(pwd)/depends/x86_64-pc-linux-gnu --enable-zmq --enable-reduce-exports --enable-crash-hooks --enable-c++20 --disable-ccache`
    * Unit tests must be run with `make check-recursive -j$(( $(nproc --all) - 2 ))`
  * An index must be initialized **after** the chain is constructed, this seems to be corroborated by all other index usage ([source](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/blockfilter_index_tests.cpp#L141), [source](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/coinstatsindex_tests.cpp#L33), [source](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/txindex_tests.cpp#L31), all three use `Start()` for their respective indexes _after_ `TestChain100Setup`'s constructor runs `mineBlocks()`)
    * Attempting to run `Start()` earlier (_before_ the `mineBlocks()` call in the constructor) results in erratic behaviour
    * This also explains why my attempt at moving it back to `TestingSetup` (a grandparent of `TestChainSetup`) failed
  * `Interrupt()` is supposed to be called before `Stop()` but this was erroneously removed in a [commit](cc9dcdd#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6L413-L419) that adopted `IndexWaitSynced`. This has since been resolved.
  * In line [with](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/blockfilter_index_tests.cpp#L138-L139) [other](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/coinstatsindex_tests.cpp#L29-L31) [indexes](https://github.com/dashpay/dash/blob/09239a17c7de643d3d45f63bfd13e08036089acc/src/test/txindex_tests.cpp#L28-L29), an sanity check has been added. Additionally, as `TxIndex::Start()` is more akin to `CChainState::LoadGenesisBlock()` than `CChainState::CanFlushToDisk()`, the `assert` has been downgraded to an exception.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  knst:
    utACK 77915de
  UdjinM6:
    utACK 77915de
  PastaPastaPasta:
    utACK 77915de

Tree-SHA512: 5051dcf62b6cad4597044b4d27dc54c5bafa8692ba31e03c8afc0f7ef80b790a3873cf0275bb2cf6260939a11f95625da79fc7987951e66114900a86276c037c
@kwvg kwvg mentioned this pull request Jul 21, 2024
Merged
5 tasks
PastaPastaPasta added a commit that referenced this pull request Jul 23, 2024
@PastaPastaPasta
Merge #6137: backport: merge bitcoin#21732, bitcoin#21762, bitcoin#21754
ef2ad76 
, bitcoin#21953, bitcoin#21850, bitcoin#22633, bitcoin#22738, bitcoin#23154, bitcoin#23721, bitcoin#24002, bitcoin#24197, merge bitcoin-core/gui#399 (auxiliary backports: part 14)

1c5ea38 merge bitcoin#24197: Replace lock with thread safety annotation in CBlockTreeDB::LoadBlockIndexGuts() (Kittywhiskers Van Gogh)
e5e3745 merge bitcoin#24002: add thread safety lock assertion to WriteBlockIndexDB() (Kittywhiskers Van Gogh)
04a3f65 merge bitcoin#23721: Move restorewallet() logic to the wallet section (Kittywhiskers Van Gogh)
e47d5ac merge bitcoin#23154: add assumeutxo notes (Kittywhiskers Van Gogh)
847d866 merge bitcoin#22738: fix failure in feature_nulldummy.py on single-core machines (Kittywhiskers Van Gogh)
ad96ef2 merge bitcoin#22633: Replace remaining binascii method calls (Kittywhiskers Van Gogh)
b37f609 merge bitcoin-core/gui#399: Fix "Load PSBT" functionality when no wallet loaded (Kittywhiskers Van Gogh)
94173f1 merge bitcoin#21850: Remove `GetDataDir(net_specific)` function (Kittywhiskers Van Gogh)
6264c7b merge bitcoin#21953: fuzz: Add utxo_snapshot target (Konstantin Akimov)
8b7ea28 merge bitcoin#21754: Run feature_cltv with MiniWallet (Kittywhiskers Van Gogh)
bd75014 merge bitcoin#21762: Speed up mempool_spend_coinbase.py (Kittywhiskers Van Gogh)
72eeb9a merge bitcoin#21732: Move common init code to init/common (Kittywhiskers Van Gogh)
3944d4e chore: resolve nit from dash#6085 (blockstorage backports) (Kittywhiskers Van Gogh)
92509e2 fix: don't suppress `-logtimestamps` help if `HAVE_THREAD_LOCAL` undef (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependency for #6138

  * In [bitcoin#21754](bitcoin#21754), the `scriptSig` padding multiplier (`24`) differs from upstream (`35`) as the `vsize` value it corresponds to must match what is ordinarily generated (`85` vs `96` upstream) in order to fulfill an assertion ([source](https://github.com/dashpay/dash/blob/d9835515cc1e1fd7d4fd3006b51a141fa2265613/test/functional/test_framework/wallet.py#L107)).

  * In [bitcoin#21953](bitcoin#21953), the hash associated with height `200` is generated like this (this is the same method used in [dash#5236](#5236)):
    * Add the height desired to the `CRegTestParams::m_assumeutxo_data` map with a garbage hash value (like `uint256::ONE`). This is to avoid an unrecognized metadata failure ([source](https://github.com/dashpay/dash/blob/5211886fb44c839d9197599d39908e1b707dfc7c/src/validation.cpp#L5755-L5761)) caused by looking through the map to see if the height's there.
    * Change the `LogPrintf(..)` in the serialized hash check error log message located [here](https://github.com/dashpay/dash/blob/5211886fb44c839d9197599d39908e1b707dfc7c/src/validation.cpp#L5876-L5880) to a `std::cout << strprintf(..)`
    * Edit the value of `mineBlocks` [here](https://github.com/dashpay/dash/blob/5211886fb44c839d9197599d39908e1b707dfc7c/src/test/validation_chainstatemanager_tests.cpp#L248-L253) to be 100 blocks _less_ than the desired height.
    * Compile Dash Core and run `./src/test/test_dash -t validation_chainstatemanager_tests`
    * Take the `got` value printed to your terminal window/`stdout` (the `expected` value should be our garbage value from earlier, ignore that). That's your good hash.
    * Update the `CRegTestParams::m_assumeutxo_data` map entry with the correct entry, reverse every change _except_ the map entry (for obvious reasons) and the `mineBlocks` change.
      * Remember to add/update the hash [here](https://github.com/dashpay/dash/blob/5211886fb44c839d9197599d39908e1b707dfc7c/src/test/validation_tests.cpp#L29-L31) in `validation_tests`, it simply tests the hardcoded chainparams value with its own hardcoded value. That's also why we don't use this test since it'll just regurgitate the garbage values we give it.
    * Compile and re-run the test. If it passes, your hash is good. Revert the `mineBlocks` change.
    * Profit?

  ## Breaking Changes

  None expected.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 1c5ea38
  PastaPastaPasta:
    utACK 1c5ea38

Tree-SHA512: 1ce0d4f1cef68990412e2e7046b36db7425059ee41b39e3681fa05d59fe24a0a74ad8c5d833c0e4c0686f693af665ca749e504b88ad30e708fc163045160aa58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@knst knst Awaiting requested review from knst

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
20
Development

Successfully merging this pull request may close these issues.
4 participants
@kwvg @knst @UdjinM6 @PastaPastaPasta