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.

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

[0.18] Revert GetData randomization change (#14897) #15839

Merged
merged 1 commit into from Apr 18, 2019

Conversation

Projects
None yet
9 participants
@sdaftuar
Copy link
Member

commented Apr 17, 2019

This is for 0.18, not master -- I propose we revert the getdata change for the 0.18 release, rather than try to continue patching it up. It seems like we've turned up several additional bugs that slipped through initial review (see #15776, #15834), and given the potential severe consequences of these bugs I think it'd make more sense for us to delay releasing this code until 0.19.

Since the bugfix PRs are getting review, I think we can leave #14897 in master, but we can separately discuss if it should be reverted in master as well if anyone thinks that would be more appropriate.

@naumenkogs

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

I think reverting for 0.18 is reasonable.

Regarding reverting in master: it seems like you're the only person working on that chunk of code, so keeping it there perhaps won't hurt for now? I'd propose just to spend more time looking for potential issues there.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

sad ACK for 0.18 (😢)

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

utACK. We should be more liberal with reverting stuff when issues are found before release. And if we're sad about delays in getting fixes out, more aggressive at backporting working changes...

@fanquake fanquake added this to the 0.18.0 milestone Apr 17, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Unless there are objections, this will be merged before todays meeting, so that rc4 can be tagged before the weekend.

@jamesob

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Concept ACK

@laanwj

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

I tend to agree this is the best solution for now. Could always backport the randomization again for 0.18.X[X>0] if there's more confidence in it then.

@MarcoFalke MarcoFalke added Backport and removed Needs backport labels Apr 18, 2019

@Empact

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Concept ACK

@MarcoFalke MarcoFalke merged commit 8602d8b into bitcoin:0.18 Apr 18, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Apr 18, 2019

Merge #15839: [0.18] Revert GetData randomization change (#14897)
8602d8b Revert "Change in transaction pull scheduling to prevent InvBlock-related attacks" (Suhas Daftuar)

Pull request description:

  This is for 0.18, not master -- I propose we revert the getdata change for the 0.18 release, rather than try to continue patching it up.  It seems like we've turned up several additional bugs that slipped through initial review (see #15776, #15834), and given the potential severe consequences of these bugs I think it'd make more sense for us to delay releasing this code until 0.19.

  Since the bugfix PRs are getting review, I think we can leave #14897 in master, but we can separately discuss if it should be reverted in master as well if anyone thinks that would be more appropriate.

ACKs for commit 8602d8:

Tree-SHA512: 0389cefc1bc74ac47f87866cf3a4dcfe202740a1baa3db074137e0aa5859672d74a50a34ccdb7cf43b3a3c99ce91e4ba1fb512d04d1d383d4cc184a8ada5543f

MarcoFalke added a commit that referenced this pull request Apr 26, 2019

Merge #15852: doc: 0.18: Remove TODO from release notes
fad9eb1 doc: 0.18: Remove TODO from release notes (MarcoFalke)

Pull request description:

  Also, remove section that no longer applies after #15839

ACKs for commit fad9eb:

Tree-SHA512: 259cbaa313cd6cce0f83a577527612314b484e77c7001fe56a21553dc0e446d9aa2bdf1129e546f9c4c142fa3cf4479d72d4561ee553a8a6e9d5453061813a3c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.