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#22840, #22937, #23446, #23522, #24026, #24104, #24167, #20744, partial bitcoin#23469, #24169 (replace boost::filesystem with std::filesystem) #6138

Merged
merged 11 commits into from
Aug 7, 2024
Merged

backport: merge bitcoin#22840, #22937, #23446, #23522, #24026, #24104, #24167, #20744, partial bitcoin#23469, #24169 (replace boost::filesystem with std::filesystem) #6138

merged 11 commits into from
Aug 7, 2024

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Jul 21, 2024 •
edited
Loading

Additional Information

Breaking Changes

None observed.

Checklist:

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

This was referenced Jul 21, 2024
Merged
Merged
@kwvg kwvg added this to the 21.1 milestone Jul 21, 2024
PastaPastaPasta added a commit that referenced this pull request Jul 23, 2024
@PastaPastaPasta
Merge #6085: backport: merge bitcoin#21727, bitcoin#22371, bitcoin#21526
1e9694d 
, bitcoin#23174, bitcoin#23785, bitcoin#23581, bitcoin#23974, bitcoin#22932, bitcoin#24050, bitcoin#24515 (blockstorage backports)

1bf0bf4 merge bitcoin#24515: Only load BlockMan in BlockMan member functions (Kittywhiskers Van Gogh)
5c1eb67 merge bitcoin#24050: Give m_block_index ownership of CBlockIndexes (Kittywhiskers Van Gogh)
c440304 merge bitcoin#22932: Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main (Kittywhiskers Van Gogh)
e303a4e merge bitcoin#23974: Make blockstorage globals private members of BlockManager (Kittywhiskers Van Gogh)
301163c merge bitcoin#23581: Move BlockManager to node/blockstorage (Kittywhiskers Van Gogh)
732e871 merge bitcoin#23785: Move stuff to ChainstateManager (Kittywhiskers Van Gogh)
b402fd5 merge bitcoin#23174: have LoadBlockIndex account for snapshot use (Kittywhiskers Van Gogh)
a08f2f4 merge bitcoin#21526: UpdateTip/CheckBlockIndex assumeutxo support (Kittywhiskers Van Gogh)
472caa0 merge bitcoin#22371: Move pblocktree global to BlockManager (Kittywhiskers Van Gogh)
d69ca83 merge bitcoin#21727: Move more stuff to blockstorage (Kittywhiskers Van Gogh)
6df927f chore: exclude underscore placeholder from shadowing linter warnings (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on #6078

  * Dependent on #6074

  * Dependent on #6083

  * Dependent on #6119

  * Dependency for #6138

  * In [bitcoin#24050](bitcoin#24050), `BlockMap` is given ownership of the `CBlockIndex` instance contained within the `unordered_map`. The same has not been done for `PrevBlockMap` as `PrevBlockMap` is populated with `pprev` pointers and doing so seems to break validation logic.

  * Dash has a specific linter for all Dash-specific code present in Core. The introduction of `util/translation.h` into `validation.h` has caused the linter to trigger shadowing warnings due to a conflict between the common use of `_` as a placeholder/throwaway name ([source](https://github.com/dashpay/dash/blob/37e026a038a60313214e01b6aba029809ea7ad39/src/spork.cpp#L44)) and upstream's usage of it to process translatable strings ([source](https://github.com/dashpay/dash/blob/37e026a038a60313214e01b6aba029809ea7ad39/src/util/translation.h#L55-L62)).

    Neither C++17 nor C++20 have an _official_ placeholder/throwaway term or annotation for structured bindings (which cannot use `[[maybe_unused]` or `std::ignore`) but [P2169](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2169r4.pdf) is a proposal put forth to make it the official placeholder, in that light, the linter will silence shadowing warnings involving an underscore.

  ## 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 1bf0bf4 (with one nit)
  knst:
    utACK 1bf0bf4
  PastaPastaPasta:
    utACK 1bf0bf4

Tree-SHA512: 875fff34fe91916722f017526135697466e521d7179c473a5c0c444e3aa873369019b804dee9f5f795fc7ebed5c2481b5ce2d895b2950782a37de7b098157ad4
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please rebase.

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
@kwvg kwvg mentioned this pull request Jul 24, 2024
Merged
5 tasks
@kwvg kwvg marked this pull request as ready for review July 26, 2024 10:15
UdjinM6
UdjinM6 previously approved these changes Jul 26, 2024
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.

utACK d0887c3

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@kwvg kwvg requested a review from knst August 2, 2024 08:08
knst
knst reviewed Aug 6, 2024
View reviewed changes
src/qt/bitcoin.cpp Outdated Show resolved Hide resolved
knst
knst reviewed Aug 6, 2024
View reviewed changes
src/qt/rpcconsole.cpp Show resolved Hide resolved
src/test/fs_tests.cpp Outdated Show resolved Hide resolved
doc/build-unix.md Show resolved Hide resolved
src/interfaces/wallet.h Show resolved Hide resolved
src/fs.cpp Outdated Show resolved Hide resolved
src/util/system.cpp Show resolved Hide resolved
@kwvg kwvg changed the title backport: merge bitcoin#22840, #22937, #23446, #23469, #23522, #24026, #24104, #24167, #20744, partial bitcoin#24169 (replace boost::filesystem with std::filesystem) backport: merge bitcoin#22840, #22937, #23446, #23522, #24026, #24104, #24167, #20744, partial bitcoin#23469, #24169 (replace boost::filesystem with std::filesystem) Aug 6, 2024
knst
knst approved these changes Aug 6, 2024
View reviewed changes
Copy link
Collaborator

@knst knst left a comment •
edited
Loading

Choose a reason for hiding this comment

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

utACK 0f23920

git range-diff 0f239203a890494b3f147c9060f039d7c2eb5c37~11..0f239203a890494b3f147c9060f039d7c2eb5c37 e754fa21b9764adb75a1027680027dfbc57897c4~11..e754fa21b9764adb75a1027680027dfbc57897c4

c4~11..e754fa21b9764adb75a1027680027dfbc57897c4
 1:  28b96a071d =  1:  4a4627f839 merge bitcoin#22840: fix unoptimized libraries in depends
 2:  23fe7e2f07 =  2:  41793c307c chore: dashify symbols in some unit tests
 3:  ecfac10b8e =  3:  f0194be1fc merge bitcoin#22937: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method
 4:  193f6fde2e =  4:  9bf02baf8a merge bitcoin#23446: Mention that BerkeleyDB is for legacy wallet in build-unix
 5:  20d359b570 =  5:  9a678e837f partial bitcoin#23469: Remove Boost build note from build-unix.md
 6:  b0d2484a0b =  6:  5f53ba8c69 merge bitcoin#23522: Improve fs::PathToString documentation
 7:  7c270e6883 =  7:  bca606da6b merge bitcoin#24026: Block unsafe std::string fs::path conversion copy_file calls
 8:  7ffea4348f =  8:  02a3fcb7ca merge bitcoin#24104: Make compatible with boost 1.78
 9:  be7ac493d0 =  9:  25ece7e2bd merge bitcoin#24167: consistently use fsbridge:: for ifstream / ofstream
10:  a3b79267e0 = 10:  15d418e00b merge bitcoin#20744: Use std::filesystem. Remove Boost Filesystem & System
11:  0f239203a8 = 11:  e754fa21b9 partial bitcoin#24169: Add --enable-c++20 option

@kwvg kwvg requested a review from UdjinM6 August 6, 2024 19:27
PastaPastaPasta
PastaPastaPasta approved these changes Aug 7, 2024
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 0f23920

@PastaPastaPasta PastaPastaPasta merged commit 87c918a into dashpay:develop Aug 7, 2024
5 of 9 checks passed
@UdjinM6 UdjinM6 modified the milestones: 21.1, 21.2 Aug 8, 2024
@kwvg kwvg mentioned this pull request Aug 10, 2024
Merged
5 tasks
PastaPastaPasta added a commit that referenced this pull request Aug 20, 2024
@PastaPastaPasta
Merge #6150: backport: merge bitcoin#24252, bitcoin#24301, bitcoin#25808
ada8591 
, bitcoin#25873, bitcoin#26557, bitcoin#25465, bitcoin#26952, bitcoin#25898:, bitcoin#30217, bitcoin#24266, partial bitcoin#28622 (replace boost::filesystem with std::filesystem: part 2)

f4b896e merge bitcoin#24266: Avoid buggy std::filesystem:::create_directories() call (Kittywhiskers Van Gogh)
b02d5e3 merge bitcoin#30217: Update Boost download link (Kittywhiskers Van Gogh)
9e80893 partial bitcoin#28622: use macOS 14 SDK (Xcode 15.0) (Kittywhiskers Van Gogh)
05fb206 merge bitcoin#25898: remove WSL 1 workaround in fs (Kittywhiskers Van Gogh)
da5b433 merge bitcoin#26952: Avoid `BOOST_NO_CXX98_FUNCTION_BASE` macro redefinition (Kittywhiskers Van Gogh)
7a1f48e merge bitcoin#25465: remove boost library detection (Kittywhiskers Van Gogh)
1d8b890 merge bitcoin#26557: Update Boost to 1.81.0 in depends (Kittywhiskers Van Gogh)
1ad64da merge bitcoin#25873: Boost 1.80.0 (Kittywhiskers Van Gogh)
d2c968b merge bitcoin#25808: work around u8path deprecated-declaration warnings with libc++ (Kittywhiskers Van Gogh)
aa361b2 merge bitcoin#24301: header-only Boost (Kittywhiskers Van Gogh)
357d1b6 merge bitcoin#24252: Represent paths with fs::path instead of std::string (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Depends on #6138

  ## Breaking Changes

  None observed.

  ## 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 f4b896e

Tree-SHA512: a23b97a4e2ff5749b8b964a76fab40aa670d1b2c43debdd125d9a747f7e411523b080dc222ef5e2c6dcd9b190afd757392c33eb9e003dc1408d33f52b2e0ecb6
@UdjinM6 UdjinM6 modified the milestones: 21.2, 22 Oct 29, 2024
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

@knst knst knst approved these changes

@UdjinM6 UdjinM6 Awaiting requested review from UdjinM6

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

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