chain: disable goosig after 1 year + 1 month #305

tynes · 2019-11-20T17:20:39Z

Edited 1/24/20

Adds a new hard coded deployment that disables GooSig after 1 year and 1 month of blocktime. This is to prevent the cryptography being broken in an unforseen way or a social attack where somebody claims it is broken when it actually isn't.

~~Two~~ One new field is added to lib/protocol/networks

~~[network].airdrop.disableGoosig~~
~~[network].airdrop.disableGoosigHeight~~
[network].goosigStop

I used 2 variables to be very explicit, disableGoosig is a boolean and disableGoosigHeight is an integer. We could combine them into a single variable using -1 as disableGoosig: false if that is preferred.

~~The DeploymentState is updated with a new property goobye and a getter hasGooBye. I wasn't sure what the best name for this deployment is, so I just went with that. Open to suggestions.~~

The functionality is as of follows:

If the network allows for GooSig to be disabled and the height of the previous block is greater than or equal to the hardcoded network height, then disable GooSig.

TODO:

Figure out correct testnet values

codecov-io · 2019-11-20T17:24:11Z

Codecov Report

Merging #305 into master will increase coverage by 0.44%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #305      +/-   ##
==========================================
+ Coverage   58.71%   59.15%   +0.44%     
==========================================
  Files         129      130       +1     
  Lines       35802    35970     +168     
  Branches     6028     6046      +18     
==========================================
+ Hits        21022    21279     +257     
+ Misses      14780    14691      -89

Impacted Files	Coverage Δ
lib/protocol/network.js	`77.41% <100%> (+0.14%)`	⬆️
lib/protocol/networks.js	`100% <100%> (ø)`	⬆️
lib/mempool/mempool.js	`46.82% <42.85%> (+3.99%)`	⬆️
lib/node/spvnode.js	`48.95% <75%> (ø)`
lib/blockchain/chain.js	`61.4% <83.33%> (+0.95%)`	⬆️
lib/covenants/rules.js	`73.04% <0%> (-0.15%)`	⬇️
lib/script/script.js	`59.32% <0%> (-0.09%)`	⬇️
lib/blockchain/chaindb.js	`64.02% <0%> (+0.18%)`	⬆️
lib/mempool/fees.js	`66.42% <0%> (+0.24%)`	⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 733e111...2d10707. Read the comment docs.

lib/protocol/networks.js

kilpatty · 2019-11-20T17:48:57Z

If I'm understanding this correctly, does this essentially put a time limit on the airdrops claims of 1 year and 1 month? I instinctively feel that this is a bit too short, but definitely open to setting *some time limit on Goosig.

tynes · 2019-11-20T17:50:59Z

If I'm understanding this correctly, does this essentially put a time limit on the airdrops claims of 1 year and 1 month? I instinctively feel that this is a bit too short, but definitely open to setting *some time limit on Goosig.

Not airdrop claims, just GooSig based claims. People will still be able to claim without using GooSig by using a normal RSA signature after the time period has expired.

kilpatty · 2019-11-20T17:56:37Z

Not airdrop claims, just GooSig based claims. People will still be able to claim without using GooSig by using a normal RSA signature after the time period has expired.

If Goosig is broken, doesn't this result in the same outcome privacy wise?

EDIT: (This only applies to RSA/Goosig based claims)
Goosig not broken && lasts forever -> Goosig claims are fine.
Goosig broken && lasts forever -> Goosig claims are de-anonymized.
Goosig not broken && removed after 1 year -> Early Airdrop claimers are fine, new ones are de-anonymized.
Goosig broken && removed after 1 year -> Everyone is de-anonymized.

Does removing Goosig stop the crypto being broken retroactively?

EDIT: I'm understanding now that de-anonymization may not be the only worry of the cryptography being broken. I think this makes a lot more sense in that context so feel free to ignore the above scenarios.

tynes · 2019-11-20T20:37:17Z

If Goosig is broken, doesn't this result in the same outcome privacy wise?

You also need to consider unforgeability, the event by which somebody without the private key can create a signature that is valid.

tynes · 2019-11-20T21:22:48Z

Updated the mempool code as well to not allow goosig based airdrops into the mempool after the network defined number of blocks has passed. A peer will be banned if they gossip a goosig based airdrop.

kilpatty · 2019-11-20T21:27:55Z

@tynes since regtest essentially allows for an arbitrary number of blocks to be mined at any point, should we consider enabling this on it as well with similar values to testnet?

tynes · 2019-11-20T21:32:24Z

@kilpatty I thought purposefully not disabling GooSig on regtest would be a feature, as people may want to be able to test over time without needing to roll over their chaindb (same with simnet). If there is consensus on also disabling on regtest, I will push up a change for that.

kilpatty · 2019-11-20T21:41:20Z

Good point, and I suppose if someone really wants to test the reverse behavior, they can just adjust the values for their own local regtest network.

Quick question: What happens in the case of a node that sends Goosig airdrop proofs 1 block away from the cutoff. Those proofs are then not mined in the cutoff block, and so they become invalid. Does that peer get banned?

tynes · 2019-11-21T03:38:51Z

Quick question: What happens in the case of a node that sends Goosig airdrop proofs 1 block away from the cutoff. Those proofs are then not mined in the cutoff block, and so they become invalid. Does that peer get banned?

Good thinking. The peer would not get banned, but that transaction would be stuck in the mempool. It seems like it would be ideal to evict any goosig based airdrops from the mempool after goosig has been disabled. That way there is no accidental banning of peers.

tynes · 2019-11-21T18:10:46Z

Thinking about how to drop GooSig based airdrops out of the mempool correctly, it feels wasteful to check for the GooSig disabled deployment and then attempt to remove all GooSig airdrops every block that is added to the mempool, since no more GooSig based airdrops will be added after a certain height. Another approach would be to only drop GooSig based airdrops out of the mempool if the block being added is at the height of deployment being activated. This seems like its subject to weird race conditions with reorgs.

My main concern is that somebody will end up with a GooSig based airdrop in the mempool during the activation and will not be able to broadcast their normal RSA based airdrop.

tynes · 2019-11-25T22:35:11Z

I opted for removing all GooSig based airdrops from the mempool at the height that GooSig is disabled instead of checking for the deployment. This saves some extra checks for all future blocks that come after GooSig is disabled.

lib/mempool/mempool.js

pinheadmz · 2020-01-17T20:10:56Z

Just thinking about the UX of a user sending a goosig airdrop after the expiration time. With a Full Node, it won't go out (err.score === 100 here)

hsd/lib/node/fullnode.js

Lines 416 to 420 in c554a6f

    
           async sendAirdrop(proof) { 
        
             try { 
        
               await this.mempool.addAirdrop(proof); 
        
             } catch (err) { 
        
               if (err.type === 'VerifyError' && err.score === 0) {

But SPV has no such protection:

hsd/lib/node/spvnode.js

Lines 343 to 345 in c554a6f

    
           sendAirdrop(proof) { 
        
             return this.broadcast(proof); 
        
           }

In this case, an SPV node could be aware of the rules it is breaking since its just a chain height. I wonder if we should protect such users from being banned this way.

Probably we should update the README in hs-airdrop as well...

pinheadmz · 2020-01-17T20:12:49Z

I think since this is consensus-critical, it'd be nice to see tests. Could be similar to #330 for the flag-height activation, and similar to #319 for clearing out the mempool.

JJ's personal Airdrop is Goosig and can be tested: https://github.com/handshake-org/hsd/blob/master/test/data/airdrop-proof.base64

lib/protocol/networks.js

tynes · 2020-01-24T10:17:59Z

This now has test coverage and is ready for another round of review. Will squash when it looks good.

tynes · 2020-01-24T10:25:32Z

But SPV has no such protection:

This is a great catch.

boymanjor

@tynes I added some comments and nits re: the test.

test/disable-goosig-test.js

lib/mempool/mempool.js

tynes · 2020-01-24T18:07:27Z

Squashed 1 commit

lib/node/spvnode.js

lib/mempool/mempool.js

boymanjor

@tynes I left a couple of more comments. This is almost ready to go.

test/disable-goosig-test.js

lib/mempool/mempool.js

lib/blockchain/chain.js

tynes · 2020-01-25T23:26:19Z

@boymanjor Any more comments on this one? I think its good to be merged.

lib/mempool/mempool.js

lib/blockchain/chain.js

boymanjor

I left one last nit. LGTM.

codecov-commenter · 2024-09-28T10:46:41Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 66.66667% with 11 lines in your changes missing coverage. Please review.

Project coverage is 59.15%. Comparing base (733e111) to head (2d10707).
Report is 1110 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/mempool/mempool.js	42.85%	8 Missing ⚠️
lib/node/spvnode.js	75.00%	2 Missing ⚠️
lib/blockchain/chain.js	83.33%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #305      +/-   ##
==========================================
+ Coverage   58.71%   59.15%   +0.44%     
==========================================
  Files         129      130       +1     
  Lines       35802    35970     +168     
  Branches     6028     6046      +18     
==========================================
+ Hits        21022    21279     +257     
+ Misses      14780    14691      -89

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kilpatty reviewed Nov 20, 2019

View reviewed changes

lib/protocol/networks.js Outdated Show resolved Hide resolved

kilpatty mentioned this pull request Nov 20, 2019

in progress: Experimental Testnet #303

Closed

2 tasks

tynes force-pushed the disable-goosig branch 3 times, most recently from ac97631 to d3125c0 Compare November 20, 2019 21:19

tynes marked this pull request as ready for review November 20, 2019 21:24

tynes commented Dec 5, 2019

View reviewed changes

lib/mempool/mempool.js Outdated Show resolved Hide resolved

pinheadmz reviewed Jan 17, 2020

View reviewed changes

lib/mempool/mempool.js Outdated Show resolved Hide resolved

pinheadmz reviewed Jan 17, 2020

View reviewed changes

lib/protocol/networks.js Outdated Show resolved Hide resolved

boymanjor added the pre mainnet label Jan 23, 2020

boymanjor reviewed Jan 24, 2020

View reviewed changes

lib/protocol/networks.js Outdated Show resolved Hide resolved

tynes force-pushed the disable-goosig branch from 421983c to 1184fb2 Compare January 24, 2020 10:06

tynes added the ready for review label Jan 24, 2020

boymanjor reviewed Jan 24, 2020

View reviewed changes

pinheadmz mentioned this pull request Jan 24, 2020

[consensus] No transactions for the first two weeks #330

Merged

pinheadmz reviewed Jan 24, 2020

View reviewed changes

lib/mempool/mempool.js Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

tynes force-pushed the disable-goosig branch from 5b40d18 to 89435ca Compare January 24, 2020 18:06

tynes force-pushed the disable-goosig branch from a414bf2 to ee25ca2 Compare January 24, 2020 18:33

tynes commented Jan 24, 2020

View reviewed changes

lib/node/spvnode.js Outdated Show resolved Hide resolved

tynes force-pushed the disable-goosig branch from b5310dc to a881dbf Compare January 24, 2020 19:26

tynes commented Jan 24, 2020

View reviewed changes

lib/mempool/mempool.js Outdated Show resolved Hide resolved

boymanjor reviewed Jan 24, 2020

View reviewed changes

test/disable-goosig-test.js Outdated Show resolved Hide resolved

test/disable-goosig-test.js Outdated Show resolved Hide resolved

lib/mempool/mempool.js Outdated Show resolved Hide resolved

lib/blockchain/chain.js Outdated Show resolved Hide resolved

tynes force-pushed the disable-goosig branch from 094c420 to 1a8dc86 Compare January 25, 2020 02:30

consensus: disable goosig after 1 year + 1 month

9c092c0

tynes force-pushed the disable-goosig branch from 1a8dc86 to 9c092c0 Compare January 25, 2020 02:33

test: bad block added to invalid cache

a15beab

chjj reviewed Jan 26, 2020

View reviewed changes

lib/mempool/mempool.js Outdated Show resolved Hide resolved

mempool: do not ban peers for relaying after goostop

a95a8f4

tynes force-pushed the disable-goosig branch from a8b43e8 to a95a8f4 Compare January 26, 2020 04:32

boymanjor reviewed Jan 26, 2020

View reviewed changes

lib/blockchain/chain.js Outdated Show resolved Hide resolved

boymanjor approved these changes Jan 26, 2020

View reviewed changes

blockchain: fix bad goosig proof error message

2d10707

chjj merged commit 6bb2220 into handshake-org:master Jan 26, 2020

pinheadmz mentioned this pull request Jan 29, 2021

[Money Supply] Airdrop tree is unverifiable and impossible to audit #549

Open

eggry mentioned this pull request Jan 2, 2023

[Money Supply] Try to audit the airdrop, and find duplicate addresses and keys in airdrop trees #786

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chain: disable goosig after 1 year + 1 month #305

chain: disable goosig after 1 year + 1 month #305

tynes commented Nov 20, 2019 •

edited

Loading

codecov-io commented Nov 20, 2019 •

edited

Loading

kilpatty commented Nov 20, 2019

tynes commented Nov 20, 2019

kilpatty commented Nov 20, 2019 •

edited

Loading

tynes commented Nov 20, 2019

tynes commented Nov 20, 2019

kilpatty commented Nov 20, 2019

tynes commented Nov 20, 2019

kilpatty commented Nov 20, 2019

tynes commented Nov 21, 2019

tynes commented Nov 21, 2019

tynes commented Nov 25, 2019

pinheadmz commented Jan 17, 2020 •

edited

Loading

pinheadmz commented Jan 17, 2020 •

edited

Loading

tynes commented Jan 24, 2020

tynes commented Jan 24, 2020

boymanjor left a comment

This comment has been minimized.

tynes commented Jan 24, 2020 •

edited

Loading

boymanjor left a comment

tynes commented Jan 25, 2020

boymanjor left a comment

codecov-commenter commented Sep 28, 2024

chain: disable goosig after 1 year + 1 month #305

chain: disable goosig after 1 year + 1 month #305

Conversation

tynes commented Nov 20, 2019 • edited Loading

codecov-io commented Nov 20, 2019 • edited Loading

Codecov Report

kilpatty commented Nov 20, 2019

tynes commented Nov 20, 2019

kilpatty commented Nov 20, 2019 • edited Loading

tynes commented Nov 20, 2019

tynes commented Nov 20, 2019

kilpatty commented Nov 20, 2019

tynes commented Nov 20, 2019

kilpatty commented Nov 20, 2019

tynes commented Nov 21, 2019

tynes commented Nov 21, 2019

tynes commented Nov 25, 2019

pinheadmz commented Jan 17, 2020 • edited Loading

pinheadmz commented Jan 17, 2020 • edited Loading

tynes commented Jan 24, 2020

tynes commented Jan 24, 2020

boymanjor left a comment

Choose a reason for hiding this comment

This comment has been minimized.

tynes commented Jan 24, 2020 • edited Loading

boymanjor left a comment

Choose a reason for hiding this comment

tynes commented Jan 25, 2020

boymanjor left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 28, 2024

Codecov Report

tynes commented Nov 20, 2019 •

edited

Loading

codecov-io commented Nov 20, 2019 •

edited

Loading

kilpatty commented Nov 20, 2019 •

edited

Loading

pinheadmz commented Jan 17, 2020 •

edited

Loading

pinheadmz commented Jan 17, 2020 •

edited

Loading

tynes commented Jan 24, 2020 •

edited

Loading