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: bitcoin#21373, #21377, #21681, #21714, #21749, #21792, #21814, #21874, #21884, bitcoin-core/gui#284 #5985

Merged
merged 11 commits into from
Apr 24, 2024
Merged

backport: bitcoin#21373, #21377, #21681, #21714, #21749, #21792, #21814, #21874, #21884, bitcoin-core/gui#284 #5985

merged 11 commits into from
Apr 24, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Apr 16, 2024 •
edited

Issue being fixed or feature implemented

Regular batch of backports from bitcoin v22

What was done?

Implemented new commandline argument -bip147height for RegTest.
Backports:

How Has This Been Tested?

Run unit/functional tests

Breaking Changes

n/a

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

@knst knst changed the title backport: bitcoin#21373, #21377, #21681, #21714, #21749, #21792, #21814, #21867, #21874, #21884, bitcoin-core/gui#284 backport: bitcoin#21373, #21377, #21681, #21714, #21749, #21792, #21814, #21874, #21884, bitcoin-core/gui#284 Apr 16, 2024
@knst knst added this to the 21 milestone Apr 16, 2024
UdjinM6
UdjinM6 requested changes Apr 16, 2024
View reviewed changes
src/chainparamsbase.cpp Outdated Show resolved Hide resolved
src/chainparamsbase.cpp Show resolved Hide resolved
@knst knst requested a review from UdjinM6 April 17, 2024 07:02
UdjinM6
UdjinM6 previously approved these changes Apr 17, 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

@knst
Copy link
Collaborator Author

knst commented Apr 22, 2024

p2p_timeouts.py | ✖ Failed | 13 s

restarted - p2p_timeouts.py fails intermittently

@github-actions GitHub Actions
Copy link

This pull request has conflicts, please rebase.

@UdjinM6
Copy link

UdjinM6 commented Apr 23, 2024

Looks good but needs another rebase to fix tests

MarcoFalke and others added 11 commits April 23, 2024 22:41
@knst
Merge bitcoin#21792: test: Fix intermittent issue in p2p_segwit.py
b2d8893 
fad6269 test: Assert that exit code indicates failure (MarcoFalke)
faecb72 test: Fix intermittent issue in p2p_segwit.py (MarcoFalke)

Pull request description:

  Calling `start_node` might call `wait_for_rpc_connection`, which will fail.

  https://cirrus-ci.com/task/5669555591708672?logs=ci#L3504

  ```
    File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/p2p_segwit.py", line 1974, in test_upgrade_after_activation
      self.start_node(2, extra_args=["-reindex", f"-segwitheight={SEGWIT_HEIGHT}"])
    File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 508, in start_node
      node.wait_for_rpc_connection()
    File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 224, in wait_for_rpc_connection
      raise FailedToStartError(self._node_msg(
  test_framework.test_node.FailedToStartError: [node 2] bitcoind exited with status 1 during initialization

ACKs for top commit:
  jnewbery:
    ACK fad6269
  dhruv:
    ACK fad6269

Tree-SHA512: 4c5e39ce25e135717ea433258518f93f09d1c528c4538a8627d3da13bc0c0ba4b45911703c26392ff0f5e0cb7831a6c7cc53e6e29102d3da9c8cfce7cef333cc
@hebasto @knst
Merge bitcoin-core/gui#284: refactor: Simplify SendCoinsDialog::updat…
a63f9c3 
…eCoinControlState

5f438d6 refactor, qt: Simplify SendCoinsDialog::updateCoinControlState (João Barbosa)

Pull request description:

  This PR doesn't change behaviour, removes the coin control argument from `updateCoinControlState` since it's a class member.

ACKs for top commit:
  hebasto:
    ACK 5f438d6, I have reviewed the code and it looks OK, I agree it can be merged.
  jonatack:
    Code review ACK 5f438d6
  kristapsk:
    utACK 5f438d6. Code looks correct.

Tree-SHA512: 14abaa3d561f8c8854fed989b6aca886dcca42135880bac76070043f61c0042ec8967f2b83e50bbbb82050ef0f074209e97fa300cb4dc51ee182316e0846506d
@knst
Merge bitcoin#21714: refactor: Drop CCoinControl::SetNull
de4d2a8 
c5a470e refactor: Drop CCoinControl::SetNull (João Barbosa)

Pull request description:

  The only external call to `SetNull` is changed as follow

  ```diff
  - m_coin_control->SetNull();
  + m_coin_control = std::make_unique<CCoinControl>();
  ```

ACKs for top commit:
  theStack:
    Code-Review ACK c5a470e
  MarcoFalke:
    review ACK c5a470e 🍤

Tree-SHA512: 2588828720cdcf405fc4fb06f2aa17ad4eef2a645e12d820311006127e732258dd084993f17f23742f8e7f782e18289a6199dcec3690efc9b92458f90b816a8f
@knst
Merge bitcoin#21814: test: Fix feature_config_args.py intermittent issue
71f23d6 
fab1eb6 test: Fix feature_config_args.py intermittent issue (MarcoFalke)

Pull request description:

  Fix bitcoin#21448

ACKs for top commit:
  laanwj:
    Code review ACK fab1eb6

Tree-SHA512: cad9f684f43aa801d0c1cb5f1684ffa624df1216be225cea46b1389ba2b67cbd6159ffb786fe144bf1ca865623dd9a10289d4293cfabb678bdb243d4ea00734d
@knst
Merge bitcoin#21681: validation: fix ActivateSnapshot to use hardcode…
a02a2c0 
…d nChainTx

91d93aa validation: remove nchaintx from assumeutxo metadata (James O'Beirne)
931684b validation: fix ActivateSnapshot to use hardcoded nChainTx (James O'Beirne)

Pull request description:

  This fixes an oversight from the move of nChainTx from the user-supplied
  snapshot metadata into the hardcoded assumeutxo chainparams.

  Since the nChainTx is now unused in the metadata, it should be removed
  in a future commit.

  See: bitcoin#19806 (comment)

ACKs for top commit:
  Sjors:
    utACK 91d93aa
  ryanofsky:
    Code review ACK 91d93aa. No change to previous commit, just new commit removing now unused utxo snapshot field and updating tests.

Tree-SHA512: 445bdd738faf007451f40bbcf360dd1fb4675e17a4c96546e6818c12e33dd336dadd95cf8d4b5f8df1d6ccfbc4bf5496864bb5528e416cea894857b6b732140c
@knst
Merge bitcoin#21874: fuzz: Add WRITE_ALL_FUZZ_TARGETS_AND_ABORT
51633d7 
fa5cb6b fuzz: Add WRITE_ALL_FUZZ_TARGETS_AND_ABORT (MarcoFalke)

Pull request description:

  This is needed when stdout is polluted by the fuzz engine. stderr can't be used instead because it is polluted by aborting the program.

Top commit has no ACKs.

Tree-SHA512: bf0a2a6bcd964ff1f0f3ef6e7e297b4c780430c4d6312332ed99ace0e1c58243c1483fd387e39405837d39b36072dfeb9ae03d2a7aa728ad6955159754fd5766
@fanquake @knst
Merge bitcoin#21884: fuzz: Remove unused --enable-danger-fuzz-link-al…
eeec2f2 
…l option

fa27d6d fuzz: Remove unused --enable-danger-fuzz-link-all option (MarcoFalke)

Pull request description:

  Remove the unused build option, which was *dangerous* (as the name implies). Also remove the fuzzbuzz config, which was never used as part of this repo and seems redundant now that we integrate with oss-fuzz.

ACKs for top commit:
  practicalswift:
    cr ACK fa27d6d: patch looks correct and rationale makes sense
  hebasto:
    ACK fa27d6d, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: 9bd65ed6a76d13d8d9c7a88aaae30f701215d5d0619693a3115d5ec350808aaf6a1aa4737466a5b96f3948513ec4d063808fe16219818366720e247880a15177
@laanwj @knst
Merge bitcoin#21749: test: Bump shellcheck version
ecade9b 
08f3dbb test: Bump shellcheck version (Hennadii Stepanov)

Pull request description:

  The changelog for v0.7.2 is available [here](https://github.com/koalaman/shellcheck/blob/v0.7.2/CHANGELOG.md).

  Only [SC2268](https://github.com/koalaman/shellcheck/wiki/SC2268) requires to update our code.

ACKs for top commit:
  jarolrod:
    ACK  08f3dbb

Tree-SHA512: 4585cd1f4d9def2fbaafe5a2a57761288d432781eb8c6c6d37064727d7ca8fc3f35c552e6a2ffdf0820d753d4bde2c8e43e5f3f57d242f5f57591a9b1b03558d
@fanquake @knst
Merge bitcoin#21377: Speedy trial support for versionbits
5191138 
ffe33df chainparams: drop versionbits threshold to 90% for mainnnet and signet (Anthony Towns)
f054f6b versionbits: simplify state transitions (Anthony Towns)
55ac5f5 versionbits: Add explicit NEVER_ACTIVE deployments (Anthony Towns)
dd07e6d fuzz: test versionbits delayed activation (Anthony Towns)
dd85d54 tests: test versionbits delayed activation (Anthony Towns)
73d4a70 versionbits: Add support for delayed activation (Anthony Towns)
9e6b65f tests: clean up versionbits test (Anthony Towns)
5932744 tests: test ComputeBlockVersion for all deployments (Anthony Towns)
63879f0 tests: pull ComputeBlockVersion test into its own function (Anthony Towns)

Pull request description:

  BIP9-based implementation of "speedy trial" activation specification, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2021-March/018583.html

  Edge cases are tested by fuzzing added in bitcoin#21380.

ACKs for top commit:
  instagibbs:
    tACK bitcoin@ffe33df
  jnewbery:
    utACK ffe33df
  MarcoFalke:
    review ACK ffe33df 💈
  achow101:
    re-ACK ffe33df
  gmaxwell:
    ACK ffe33df
  benthecarman:
    ACK ffe33df
  Sjors:
    ACK ffe33df
  jonatack:
    Initial approach ACK ffe33df after a first pass of review, building and testing each commit, mostly looking at the changes and diffs. Will do a more high-level review iteration. A few minor comments follow to pick/choose/ignore.
  ariard:
    Code Review ACK ffe33df

Tree-SHA512: f79a7146b2450057ee92155cbbbcec12cd64334236d9239c6bd7d31b32eec145a9781c320f178da7b44ababdb8808b84d9d22a40e0851e229ba6d224e3be747c
@knst
feat: new command line argument -bip147height for bitcoin#21373
a933a60 
This command line argument is substitute for -segwitheight so far as dash does
not have a segwit feature
@knst
Merge bitcoin#21373: test: generate fewer blocks in feature_nulldummy…
7cc77f3 
… to fix timeouts, speed up

ccd976d test: use 327 fewer blocks in feature_nulldummy (Jon Atack)
68c280f test, refactor: abstract the feature_nulldummy blockheight values (Jon Atack)

Pull request description:

  The resolved timeout issue seen in the CI can be reproduced locally by running `test/functional/feature_nulldummy.py --valgrind --loglevel=debug`

  Speeds up the normal test runtime for me from 3.8 to 2.2 seconds (debug build). Thanks to Marco Falke for the approach suggestion.

ACKs for top commit:
  AnthonyRonning:
    reACK ccd976d - ran a few times with the rest of the tests and still passing for me with just the fewer block change.
  MarcoFalke:
    review ACK ccd976d 🏝

Tree-SHA512: 38339dca4276d1972e3a5a5ee436da64e9e58fd3b50b186e34b07ade9523ac4c03f6c3869c5f2a59c23b43c44f87e712f8297a01a8d83202049c081e3eeb4445
PastaPastaPasta
PastaPastaPasta approved these changes Apr 23, 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 7cc77f3

UdjinM6
UdjinM6 approved these changes Apr 23, 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

@PastaPastaPasta PastaPastaPasta merged commit 27b2e5c into dashpay:develop Apr 24, 2024
7 of 8 checks passed
@knst knst deleted the bp-v22-p7 branch April 24, 2024 15:27
@thephez thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

Assignees
No one assigned
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Milestone
21
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@knst @UdjinM6 @PastaPastaPasta @thephez @hebasto @fanquake @laanwj