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

merge bitcoin#16787, #17474, #17687, #18165, #18877, #18960, #19010, #19044, #19070: block filters #4355

Merged
merged 9 commits into from
Sep 21, 2021
Merged

merge bitcoin#16787, #17474, #17687, #18165, #18877, #18960, #19010, #19044, #19070: block filters #4355

merged 9 commits into from
Sep 21, 2021

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Aug 22, 2021

No description provided.

@kwvg kwvg changed the title bitcoin#16787, #17474, #17687, #18165, #18877, #18960, #19010, #19044, #19070: block filters merge bitcoin#16787, #17474, #17687, #18165, #18877, #18960, #19010, #19044, #19070: block filters Aug 22, 2021
@PastaPastaPasta
Copy link
Member

linter + both tests are unhappy

@kwvg
Copy link
Collaborator Author

kwvg commented Aug 22, 2021

It's a work in progress

@PastaPastaPasta PastaPastaPasta marked this pull request as draft August 22, 2021 20:17
@PastaPastaPasta
Copy link
Member

Marked as draft

@github-actions
Copy link

This pull request has conflicts, please rebase.

@kwvg kwvg marked this pull request as ready for review September 16, 2021 15:44
@PastaPastaPasta
Copy link
Member

Please rename all commits to begin with "Merge"

PastaPastaPasta
PastaPastaPasta requested changes Sep 17, 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.

LGTM, besides one thing and a comment

src/net_processing.h
@@ -21,6 +21,7 @@ static const unsigned int DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN = 100;
/** Default for BIP61 (sending reject messages) */
static constexpr bool DEFAULT_ENABLE_BIP61 = true;
static const bool DEFAULT_PEERBLOOMFILTERS = true;
static const bool DEFAULT_PEERBLOCKFILTERS = false;
Copy link
Member

Choose a reason for hiding this comment

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

We should consider what all of these defaults should be, like should normal nodes be serving bloom filters by default for example?

UdjinM6
UdjinM6 requested changes Sep 18, 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.

Few small issues

PastaPastaPasta
PastaPastaPasta approved these changes Sep 19, 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

UdjinM6
UdjinM6 approved these changes Sep 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

@UdjinM6 UdjinM6 added this to the 18 milestone Sep 21, 2021
@UdjinM6 UdjinM6 merged commit 4ffd42d into dashpay:develop Sep 21, 2021
@UdjinM6 UdjinM6 added the RPC Some notable changes to RPC params/behaviour/descriptions label Sep 28, 2021
@kwvg kwvg deleted the bfilters branch July 18, 2023 11:40
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
18
Development

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