Skip to content

merge bitcoin#13258, #14951 #4499

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
Merged
PastaPastaPasta merged 2 commits into from
Oct 21, 2021
Merged

merge bitcoin#13258, #14951 #4499

PastaPastaPasta merged 2 commits into from
Oct 21, 2021

Conversation

@vijaydasmp
Copy link

@vijaydasmp vijaydasmp commented Oct 10, 2021

No description provided.

PastaPastaPasta
PastaPastaPasta requested changes Oct 10, 2021
View reviewed changes
src/addrman.cpp Outdated
Copy link
Member

@PastaPastaPasta PastaPastaPasta Oct 10, 2021

Choose a reason for hiding this comment

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

likely shouldn't drop asmap here

src/uint256.h Outdated
Copy link
Member

@PastaPastaPasta PastaPastaPasta Oct 10, 2021

Choose a reason for hiding this comment

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

13258 please investigate why we don't have the line in addrman_tests.cpp that upstream changed

Copy link
Author

@vijaydasmp vijaydasmp Oct 10, 2021

Choose a reason for hiding this comment

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

int RandomInt(int nMax) method is not present in the current branch, that's why this change is not cherry-picked, checking for another commit that has this change to backport

Copy link
Author

@vijaydasmp vijaydasmp Oct 10, 2021

Choose a reason for hiding this comment

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

Merge bitcoin#14624: Some simple improvements to the RNG code (this change had removed RandomInt method, marked as partial in the sheet), and it is already merged,

@vijaydasmp vijaydasmp force-pushed the backport_v18_vijay_batch4_3 branch 2 times, most recently from 15b13da to 04d678a Compare October 10, 2021 01:32
@github-actions
Copy link

github-actions bot commented Oct 11, 2021

This pull request has conflicts, please rebase.

@vijaydasmp vijaydasmp force-pushed the backport_v18_vijay_batch4_3 branch from 04d678a to 6a86c95 Compare October 12, 2021 04:18
@vijaydasmp vijaydasmp force-pushed the backport_v18_vijay_batch4_3 branch from 0b23d50 to 9749c8d Compare October 13, 2021 11:01
@vijaydasmp
Copy link
Author

vijaydasmp commented Oct 13, 2021

feature_dip3_deterministicmns test is failing

UdjinM6
UdjinM6 requested changes Oct 13, 2021
View reviewed changes
@vijaydasmp vijaydasmp force-pushed the backport_v18_vijay_batch4_3 branch 2 times, most recently from 468d53e to 585f0c4 Compare October 14, 2021 10:33
UdjinM6
UdjinM6 requested changes Oct 20, 2021
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.

pls see 56ce214b0e7b589777a9d6fab9753557c95f4601 and c9d7367ab57b32c3e9dfdc0cb154c984ca601420.

@vijaydasmp vijaydasmp force-pushed the backport_v18_vijay_batch4_3 branch from 585f0c4 to 1be1049 Compare October 20, 2021 04:00
@vijaydasmp vijaydasmp marked this pull request as ready for review October 20, 2021 04:49
@vijaydasmp vijaydasmp changed the title Backport v18 vijay batch4 3 merge bitcoin#13258, #14951 Oct 21, 2021
UdjinM6
UdjinM6 requested changes Oct 21, 2021
View reviewed changes
laanwj and others added 2 commits October 21, 2021 17:41
@laanwj @vijaydasmp
Merge bitcoin#13258: uint256: Remove unnecessary crypto/common.h depe…
d0724b5 
…ndency

bf2e010 uint256: Remove unnecessary crypto/common.h use (Karl-Johan Alm)

Pull request description:

  This is an alternative to bitcoin#13242 which keeps the `ReadLE64` part, but moves the `crypto/common.h` dependency into `crypto/common.h` as a function outside of `uint256`.

  **Reason:** this change will remove dependencies for `uint256` to `crypto/common.h`, `compat/endian.h`, and `compat/byteswap.h`.

  This PR removes the need to update tests to be endian-aware/-independent, but keeps the (arguably dubious) `ReadLE64` part (which was only introduced to fix the tests, not for any functionality).

Tree-SHA512: 78b35123cdb185b3b3ec59aba5ca8a5db72624d147f2d6a5484ffa5ce626a72f782a01dc6893fc8f5619b03e2eae7b5a03b0df5d43460f3bda428e719e188aec
@vijaydasmp
Merge bitcoin#14951: Revert "tests: Support calling add_nodes more th…
b5bc278 
…an once"

fa4b8c9 test: add_nodes can only be called once after set_test_params (MarcoFalke)
faa8311 Revert "tests: Support calling add_nodes more than once" (MarcoFalke)

Pull request description:

  Writing tests should be straightforward and with little side-effects as possible.

  I don't see how this is needed and can not be achieved with `self.num_nodes` (and `self.extra_args` et al.)

Tree-SHA512: 83a67f2cba9d97e21d80847ff405a4633fcb0d5674486efa57ee1813e46efe8709ae0fb462b8339a01ebeca5c4f2d29ecb1807d648b8fd9ee8ce336b08d580a8
@vijaydasmp vijaydasmp force-pushed the backport_v18_vijay_batch4_3 branch from 1be1049 to b5bc278 Compare October 21, 2021 12:16
@vijaydasmp vijaydasmp requested a review from UdjinM6 October 21, 2021 12:32
@vijaydasmp
Copy link
Author

vijaydasmp commented Oct 21, 2021

Somebody please retrigger the test, feature_config_args this test is flaky

@vijaydasmp
Copy link
Author

vijaydasmp commented Oct 21, 2021

now feature_llmq_dkgerrors.py is failing :O

@vijaydasmp
Copy link
Author

vijaydasmp commented Oct 21, 2021

Somebody please retrigger the test, feature_config_args this test is flaky

Thanks @UdjinM6

UdjinM6
UdjinM6 approved these changes Oct 21, 2021
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 reviewed Oct 21, 2021
View reviewed changes
src/txdb.h
Comment on lines -94 to -97
private:
CCriticalSection cs;
unordered_limitedmap<uint256, bool> mapHasTxIndexCache;

Copy link
Member

@PastaPastaPasta PastaPastaPasta Oct 21, 2021

Choose a reason for hiding this comment

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

how are these changes related to 13258

Copy link

@UdjinM6 UdjinM6 Oct 21, 2021

Choose a reason for hiding this comment

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

Removing <crypto/common.h> from src/uint256.h requires dropping uint256::GetCheapHash() which requires dropping std::hash() for uint256.
Dash-specific changes (which were took from my patch):

  1. implement similar hash functions when uint256 represents block hashes.
  2. use StaticSaltedHasher in other cases, see b5462f5 commit message for "why?". NOTE: this does in fact change the behaviour for those unordered maps/sets that were switched to StaticSaltedHasher but in a good way imo.
  3. drop unused parts instead of fixing them (mapHasTxIndexCache is unused and cs which was guarding it in the old code is unused too now).

@PastaPastaPasta
Copy link
Member

PastaPastaPasta commented Oct 21, 2021

14951 is good, just a little confused with 13258

PastaPastaPasta
PastaPastaPasta approved these changes Oct 21, 2021
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

@PastaPastaPasta PastaPastaPasta merged commit dc93063 into dashpay:develop Oct 21, 2021
@UdjinM6 UdjinM6 added this to the 18 milestone Oct 21, 2021
@vijaydasmp vijaydasmp deleted the backport_v18_vijay_batch4_3 branch December 29, 2021 01:23
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

None yet

Projects

None yet

Milestone

18

Development

Successfully merging this pull request may close these issues.

4 participants

@vijaydasmp @PastaPastaPasta @UdjinM6 @laanwj