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.11] dbwrapper: Detect obfuscation #7259

Merged

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Dec 26, 2015

Replaces #6919.
Fixes #7258.
Implements #6613 (comment).

@pstratem
Copy link
Contributor

concept ACK fa24941

@luke-jr
Copy link
Member

luke-jr commented Dec 26, 2015

Won't this detect the no-op obfuscation too? That wouldn't be a good idea...

@pstratem
Copy link
Contributor

@MarcoFalke luke-jr is right you need to also check for the null obfuscation key

@maflcko
Copy link
Member Author

maflcko commented Dec 26, 2015

@pstratem Why would someone write a null obfuscation key?

@pstratem
Copy link
Contributor

@MarcoFalke it's a quirk of how the obfuscation code works, it's XOR so a null obfuscation key is the same as no obfuscation key

@sipa
Copy link
Member

sipa commented Dec 26, 2015

Do we ever write a null key? (as opposed to treating no key as null)

@maflcko
Copy link
Member Author

maflcko commented Dec 26, 2015

@pstratem But it is never written to the db. This would be like saying I obfuscate every of my GitHub messages with the null obfuscation key. (It's redundant and not needed)

@pstratem
Copy link
Contributor

@MarcoFalke I stand corrected

@laanwj
Copy link
Member

laanwj commented Jan 4, 2016

Concept ACK but doubting if this or #6919 makes more sense.
Sure, backporting the whole thing involves somewhat more code but it seemed more useful than introducing a little bit less of code to just warn.

@maflcko
Copy link
Member Author

maflcko commented Jan 4, 2016

@laanwj This can be cherry-picked to 0.10 without conflicts as well. As this issue seems popular ( #7258 (comment) ) we need either #7259 (this) or #6919, imo.

Keep in mind you can rebase #6919 simply on a commit which reverts #7259 (this), in case you want to reopen after #7259 (this) got merged. ("'Don't Let Perfect Be The Enemy Of Good'")

@laanwj
Copy link
Member

laanwj commented Jan 5, 2016

Yeah, this is fine for 0.11. It's easier to test that it rejects a database than to test whether it works.

Would be nice to have at least one tested ACK, though.

@laanwj laanwj merged commit fa3cb49 into bitcoin:0.11 Jan 9, 2016
laanwj added a commit that referenced this pull request Jan 9, 2016
fa3cb49 [init] Fix typo (MarcoFalke)
fa24941 [dbwrapper] Detect obfuscation (MarcoFalke)
@maflcko maflcko deleted the MarcoFalke-2015-dbWrapperObfuscation-0.11 branch January 9, 2016 15:52
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 10, 2016
Github-Pull: bitcoin#7259
Rebased-From: fa3cb49
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants