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

[consensus] Add opt-in transaction avoidance #117

Closed
wants to merge 1,785 commits into from

Conversation

Projects
None yet
@jgarzik
Copy link

commented Aug 23, 2017

Based on a gist by @gavinandresen at https://gist.github.com/gavinandresen/2a7474a3dd5b834ed3a7d10c74ec84c5

Transactions may contain a marker, a transaction output (txout)
constructed as: OP_RETURN 'RP=!>1x'

  1. All transactions containing this marker are considered non-standard,
    and not relayed or mined by default. They may appear in a block
    ("valid"), but the software will not do this by default. This is
    normal behavior for any valid-but-nonstandard transaction.

  2. After the BIP102 hard fork point (fSegwitSeasoned), transactions
    containing this marker are considered invalid. Blocks must not contain
    transactions with a 'RP=!>1x' marker.

jonasschnelli and others added some commits Jul 17, 2017

Remove factor of 3 from definition of dust.
This redefines dust to be the value of an output such that it would
cost that value in fees to (create and) spend the output at the dust
relay rate.  The previous definition was that it would cost 1/3 of the
value.  The default dust relay rate is correspondingly increased to
3000 sat/kB so the actual default dust output value of 546 satoshis
for a non-segwit output remains unchanged.  This commit is a refactor
only unless a dustrelayfee is passed on the commandline in which case
that number now needs to be increased by a factor of 3 to get the same
behavior.  -dustrelayfee is a hidden command line option.

Note: It's not exactly a refactor due to edge case changes in rounding
as evidenced by the required change to the unit test.
Merge bitcoin#10837: Fix resource leak on error in GetDevURandom
a8ae0b2 Fix resource leak (Dag Robole)

Pull request description:

  Fixes a potential file handle leak when size of entropy is invalid

Tree-SHA512: 692d24daaf370bba1f842925b037275126f9494f54769650bcf5829c794a0fb8561a86f42347bdf088a484e4f107bce7fa14cd7bdbfb4ecfbeb51968953da3ae
init: Factor out AppInitLockDataDirectory
Alternative to bitcoin#10818, alternative solution to bitcoin#10815.

After this change: All the AppInit steps before and inclusive
AppInitLockDataDirectory must not have Shutdown() called in case of
failure. Only when AppInitMain fails, Shutdown should be called.

Changes the GUI and bitcoind code to consistently do this.
Merge bitcoin#10834: Remove declaration of unused method: void Update…
…dTransaction(const uint256 &)

e061d8d Remove declaration of unused function: void UpdatedTransaction(const uint256 &) (practicalswift)

Pull request description:

  Remove declaration of unused method: `void UpdatedTransaction(const uint256 &)`

  Removed in 9fececb.

Tree-SHA512: a328e00ccecc2a31cda7e204ab8c29ea34811afc2090f3dfd88d6d58543761dba8a289585d30b98e05236c67b1a04844e556a3b05d6e8e706233bc502f31af1d
Pushdown walletdb though CWallet::AddKeyPubKey to avoid flushes.
This prevents the wallet from being flushed between each and
 every key during top-up.  This results in a >10x speed-up
 for the top-up.
Improve api to estimatesmartfee
Change parameter for conservative estimates to be an estimate_mode string.
Change to never return a -1 for failure but to instead omit the feerate and
return an error string.  Throw JSONRPC error on invalid nblocks parameter.
Convert named argument from nblocks to conf_target
in estimatesmartfee and estimaterawfee.  Also reuse existing bounds checking.
Merge bitcoin#10832: init: Factor out AppInitLockDataDirectory and fi…
…x startup core dump issue

dba485d init: Factor out AppInitLockDataDirectory (Wladimir J. van der Laan)

Pull request description:

  Alternative to bitcoin#10818, alternative solution to bitcoin#10815.

  After this change: All the AppInit steps before and inclusive AppInitLockDataDirectory must not have Shutdown() called in case of failure. Only when AppInitMain fails, Shutdown should be called.

  Changes the GUI and bitcoind code to consistently do this.

Tree-SHA512: 393e1a0ae05eb8e791025069e3ac4f6f3cdeb459ec63feda85d01cf6696ab3fed7632b6a0ac3641b8c7015af51d46756b5bba77f5e5f0c446f0c2dea58bbc92e
Merge bitcoin#10831: Batch flushing operations to the walletdb during…
… top up and increase keypool size.

b0e8e2d Print one log message per keypool top-up, not one per key. (Gregory Maxwell)
41dc163 Increase wallet default keypool size to 1000. (Gregory Maxwell)
30d8f3a Pushdown walletdb though CWallet::AddKeyPubKey to avoid flushes. (Gregory Maxwell)
3a53f19 Pushdown walletdb object through GenerateNewKey/DeriveNewChildKey. (Gregory Maxwell)

Pull request description:

  This carries the walletdb object from top-up through GenerateNewKey/DeriveNewChildKey/CWallet::AddKeyPubKey, which allows us to avoid the flush on destruction until the top up finishes instead of flushing the wallet for every key.

  This speeds up adding keys by well over 10x on my laptop (actually something like 17x), I wouldn't be surprised if it were an even bigger speedup on spinning rust.

  Then it increases the keypool size to 1000. I would have preferred to use 10,000 but in the case where the user creates a new wallet and then turns on encryption it seems kind of dumb to have >400KB of marked-used born unencrypted keys just laying around.

  (Thanks to Matt for cluesticking me on how to bypass the crypter spaghetti)

Tree-SHA512: 868303de38fce4c3f67d7fe133f765f15435c94b39d252d7450b5fee5c607a3cc2f5e531861a69d8c8877bf130e0ff4c539f97500a6bc0ff6d67e4a42c9385c7
No longer ever reuse keypool indexes
This fixes an issue where you could reserve a keypool entry, then
top up the keypool, writing out a new key at the given index, then
return they key from the pool. This isnt likely to cause issues,
but given there is no reason to ever re-use keypool indexes
(they're 64 bits...), best to avoid it alltogether.
Add a discard_rate
Any change output which would be dust at the discard_rate you are
willing to discard completely and add to fee (as well as continuing to
pay the fee that would have been needed for creating the change).
Prevent user from specifying conflicting parameters to fundrawtx
estimate_mode/conf_target both are overridden by feeRate, so should
not be specified together with feeRate.
Merge bitcoin#9980: Fix mem access violation merkleblock
8276e70 Adding assert to avoid a memory access violation inside of PartialMerkleTree::CalcHash() (Chris Stewart)

Pull request description:

  Fixing a possible memory access violation in CPartialMerkleTree::CalcHash().

  This can happen if we some how a merkle tree with zero txids. I don't think this can happen in practice as we only send merkle block messages on the p2p network as of now -- we cannot receive them.

  This was found with bitcoin#8469, specifically using this [generator](https://github.com/Christewart/bitcoin/blob/rapidcheck/src/test/gen/merkleblock_gen.h#L52-L77) which will cause a memory access violation on [this test case](https://github.com/Christewart/bitcoin/blob/rapidcheck/src/test/merkleblock_properties.cpp#L48).

Tree-SHA512: b95904ec45ea3f082c7722161d93ee06b24c706fbffa909a6e995ed14788aed2830f91b626da6f0347660c45874a0735dab61c9440b59c949c690af4165c83fb
Merge bitcoin#10707: Better API for estimatesmartfee RPC
06bcdb8 Convert named argument from nblocks to conf_target (Alex Morcos)
439c4e8 Improve api to estimatesmartfee (Alex Morcos)

Pull request description:

  Through 0.14 branch, the estimatesmartfee API was tagged "WARNING: This interface is unstable and may disappear or change!" and this warning is removed for 0.15, so any wanted API updates should happen now.

  The changes here are to make the additional parameter for conservative estimates a more general estimate_mode string , to omit the feerate and include an error string instead of returning -1 on error, and to do better parameter checking initially.

  ~It is only the last 2 commits, but it's built on bitcoin#10706 and bitcoin#10543~.

  See bitcoin#10707 (comment) for renaming of nblocks argument to conf_target.  Will need to be included before string freeze.

  PR description edited for clarity

Tree-SHA512: 6d8ebee8bb410e2950ffd59663eebfed8d1611d995dc935bb91e430d9da7e2f306796f45631458376027d26341c660f09e825e61748103d2f2736ec6dc3df3ae
Merge bitcoin#10855: random: only use getentropy on openbsd
077d01f random: only use getentropy on openbsd (Cory Fields)

Pull request description:

  Follow-up from bitcoin#10335. I can confirm that this fixes my issue when building against a new glibc + old linux headers for back-compat.

Tree-SHA512: a0fcf26995fbd3636f970e729a172c6e1d7c0de371e703f0653cd9776600f438ec43acd2b1eb92f2678a011968da8fbbeef8a54599434851f4c6ffe78291c172
Merge bitcoin#10795: No longer ever reuse keypool indexes
1fc8c3d No longer ever reuse keypool indexes (Matt Corallo)

Pull request description:

  This fixes an issue where you could reserve a keypool entry, then
  top up the keypool, writing out a new key at the given index, then
  return they key from the pool. This isnt likely to cause issues,
  but given there is no reason to ever re-use keypool indexes
  (they're 64 bits...), best to avoid it alltogether.

  Builds on bitcoin#10235, should probably get a 15 tag.

Tree-SHA512: c13a18a90f1076fb74307f2d64e9d80149811524c6bda259698ff2c65adaf8c6c3f2a3a07a5f4bf03251bc942ba8f5fd33a4427aa4256748c40b062991682caf
Use a prefix that makes sense for Unlimited/Classic
The `3No2x` prefix only makes sense for explicitly Segwit2x supporting nodes.
There are other nodes that support larger blocks that will follow the 2x mined chain.
Change to a more neutral `3Bit1x` prefix that also makes sense for them.
@prusnak

This comment has been minimized.

Copy link

commented Oct 1, 2017

Opt-in replay protection is not enough as it does not prevent transaction from being included in the block. And with malicious behavior of miners we see today (triggering EDA on Bitcoin Cash by not mining, then mining blocks as fast as possible with lowered difficulty), they cannot be trusted not to include these transactions in their blocks.

@jgarzik

This comment has been minimized.

Copy link
Author

commented Oct 1, 2017

@prusnak Preventing the transaction from appearing in a block is precisely what the code does.

@jgarzik

This comment has been minimized.

Copy link
Author

commented Oct 4, 2017

Superceded by #127

@jgarzik jgarzik closed this Oct 4, 2017

@ebliever

This comment has been minimized.

Copy link

commented on a3c4125 Oct 4, 2017

If the intent is to force people onto the 2X chain I think this will backfire. News and social media will explain the process for opt-in replay protection, but bitcoin users will also learn how it could have been done, and how this method burdens the legacy chain with bloated UTXO. Users will not favor the chain that is bullying them. Please reconsider and provide automatic (not opt-in) replay protection that does not burden the legacy chain.

This comment has been minimized.

Copy link
Author

replied Oct 4, 2017

@ebliever See #120

This comment has been minimized.

Copy link

replied Oct 4, 2017

A stupid, ugly hack that fills the Bitcoin blockchain with garbage.

This comment has been minimized.

Copy link

replied Oct 4, 2017

@jgarzik The patch described at #120 has some issues with future proofing, if this method is used as is your chain will need to add an ever-expanding set of version bits, as well as the current chain implementation.
These bits are not free and, eventually, something will break catastrophically due to Human error, allowing leakage between multiple implementations and chains. This is a reason why the incumbent chain implements replay protection so that we can avoid this situation; like BCH and the upcoming Oct 25th hard fork of BTG.

There is not enough time for the entire ecosystem to implement this feature as-is, nor would I expect it to. Furthermore, even if it is implemented it does not prevent the risk of partitioning in all network versions due to false version advertising by BTC1 nodes; which is reckless by anyone's standards, correct identification of a node is vitally important to the overall health of the network. Again, if you make this standard behavior then other incumbents will see this as a legitimate method of interaction with the resulting Segwit2X chain, as you will have set the standard.

This comment has been minimized.

Copy link

replied Oct 4, 2017

The 1x SegWit Core chain needs to add in some type of replay protection in order for safety of the network.

This comment has been minimized.

Copy link

replied Oct 7, 2017

@jgarzik When can we expect the revert commit to appear and a new reasonable commit to replace it, the imposed deadline is fast approaching. Talking about a moot point of only 1 real viable solution isn't really the answer. The answer is clear when there is only 1 reasonable solution.

Personally I will only support upgrades/patches/splits of this projects magnitude if it has the following criteria:

  1. Transactions on base code are not affected (base code means the code you are forking from)

  2. Transactions on the base code cannot affect the new codebase (they shouldnt be able to affect the forked code)

Now one item I think may be up for grabs here is the competition for miner attention/support. It's my belief that this should be settled by investors/users of whichever codebase they want to support and it's not up to the codebase itself if the forked code is to use the same resource pool as the base code. That to me is healthy competition. Unhealthy competition is one where the forked code literally assaults the other by forcefully taking over resources. That just is not right nor is it healthy for anyone involved. It gives the new project a bad name and the old project is unable to continue so what happens is that BOTH projects die. Do you really want that? It could not only be bad for your project, but you personally as well as it will destroy your reputation as a developer which is the path you have already set in motion and frankly I think it's probably to late to repair that. At least in my book, I absolutely would never hire you or any company associated with you for a project. Ever. You are going to have to act extremely fast to change any opinions on this matter and not just waste time "revisiting" options. I want to see action, not more talking on a subject that's already been made crystal clear to you for months.

Fix it. And fast. That would change my opinion of you but only slightly. Have a few more good fixes and we can move the opinion slider even more back to the positive side.

I think the only thing we agree on is the ability to disagree which means a fork is necessary. I understand you propose changes that the btc core group disagrees with. That doesnt invalidate your idea but it does tell you that you need a clean fork and not something so disruptive that it is viewed as a tantrum and malicious. Take litecoin for example. Thats the road you should be taking. Lee correctly forked btc and it will probably replace btc somewhere in the future if he does things that the masses view as superior. Is it a fast process? No. Is it easy? No. And it sholdnt be anyway.

This comment has been minimized.

Copy link

replied Oct 7, 2017

@Lancelight the condescending tone of your response is ridiculous and lacks much technical thought whatsoever. If you expect @jgarzik to give any weight to your feedback I'd suggest you speak to him with a bit more respect.

This comment has been minimized.

Copy link

replied Oct 7, 2017

LMAO @jheathco He deserves as much "respect" as he has given to the BTC project.... which is absolute zero (read his other comments here in github as well as his Twitter and elsewhere). The only thing he understands is hostility. Period. 95% of the planet is AGAINST his proposed method of securing the segwit2x proposal (which is not the same as all the other proposed changes segwit2x has btw, my problem is with this commit specifically and nothing else). There are reasons beyond your understanding for that. I don't care if he forks but I do care that it would destroy both chains. See, unlike you I actually have investments over 750k in this chain and I absolutely am AGAINST anything that threatens it. Take your poor ass back to dogecoin and bother someone else while the big boys hash out a very real world problem.

This comment has been minimized.

Copy link

replied Oct 7, 2017

@Lancelight not at all surprised that your level of trolling would lack an identity associated with it.

This comment has been minimized.

Copy link

replied Oct 7, 2017

@lra

This comment has been minimized.

Copy link

commented on 28ebbdb Oct 4, 2017

This commit officially makes segwit2x a trojan horse as it allows anyone to run 2x nodes in disguise. Now core needs to find a new way to detect and ban your nodes.

This comment has been minimized.

Copy link

replied Oct 4, 2017

What have you done with the old jgarzik and how much will it cost us to buy him back?

This comment has been minimized.

Copy link

replied Oct 4, 2017

@jgarzik

This commit is a pure shame for 2X and for all 2x supporting companies.
Intentionally trying to disrupt the bitcoin network will have consequences and in a lot of states you will be held accountable by law for this attack.

I can only encourage every single B2X supporter to drop the support of B2X right now, before you will be part of a crime and have to face legal actions against you and everyone who is involved in this attack.

This comment has been minimized.

Copy link

replied Oct 4, 2017

@jgarzik this is tragic. bitcoin is about consensus not covert war tactics. this is open source software and masking clients to trick other open source software is just not OK by any community standards.

This comment has been minimized.

Copy link

replied Oct 4, 2017

is anyone planning on reverting this commit back to the way it was so that all can go back and sleep in peace.

This comment has been minimized.

Copy link

replied Oct 4, 2017

That's how you can tell there is consensus for their hard-fork; Because they have to hide their nodes.

This comment has been minimized.

Copy link

replied Oct 4, 2017

I didn't realize the hate on 2X until this commit. Thanks for spoiling it!

This comment has been minimized.

Copy link

replied Oct 4, 2017

@Logerfo Thing is, this updates is to hide segwit2x support. it's just full on attack to BTC. Why hide your intentions if you're legit?

This comment has been minimized.

Copy link

replied Oct 4, 2017

@monxas I think I was not clear: I didn't realize the reason for the hate. You're right, now I do.

This comment has been minimized.

Copy link
Author

replied Oct 4, 2017

This is a privacy feature. See discussion at PR #109 for more.

This feature defaults to advertise (privacy=off).

Users may optionally disable advertising (privacy=on).

There is historical precedent with Internet browsers: https://stackoverflow.com/questions/7975996/why-does-internet-explorer-9-report-mozilla-in-useragent

Users also rightly requested privacy features like this due to past attacks:

Bitcoin Core violated the Robustness Principle as described in RFC 1122 and elsewhere. https://tools.ietf.org/html/rfc1122

@jgarzik jgarzik reopened this Oct 7, 2017

@jheathco

This comment has been minimized.

Copy link

commented Oct 7, 2017

My guess is that if this is merged we'll hear the complaints shift to the "bloating" of the 1x blockchain with the extra tx data required to split coins.

@prusnak

This comment has been minimized.

Copy link

commented Oct 7, 2017

@arisAlexis

This comment has been minimized.

Copy link

commented on 98c0af5 Oct 9, 2017

democratic

This comment has been minimized.

Copy link

replied Oct 9, 2017

And this change is done without any notification, which is unusual given your previous modifications.
Why did you removed the replay protection in the past? Of all the large code base Bitcoin codebase, only this change? What is the benefit of deleting a safe feature? What's benefit for S2X to be dangerous for other forks? Why not improve Bitcoin code for your fork, or bug fixing, or contribute something at least? Why make it just more dangerous? I'm trying to understand you. It's same as turning off ABS or airbag for your car, which makes no sense at all.

Now you're rolling back after some users complained, and spread further without proper testing.

Only the thing that I could guess, if you want to turn off the methaphor airbags, your intention is to cause unnecessary chaos in the bitcoin space? You can do just better for the progress of financial sovereignty, instead of causing havoc and chaos for personal reasons.

This comment has been minimized.

Copy link

replied Oct 9, 2017

If this is not leading up to better replay protection implementation, then this is most "rage about loosing" commit I've ever seen.

This comment has been minimized.

Copy link

replied Oct 9, 2017

@jgarzik is discussing alternatives for replay protection that he thinks will be better suited in slack. There are varying opinions on every side, though no 2x supporter is espousing the idea that 2x adds replay protection that will break compatibility with existing SPV wallets.

Feel free to join in slack with your opinions on the best optional replay protection approaches.

This comment has been minimized.

Copy link

replied Oct 9, 2017

@JaredR26 Let's remind everyone the secret @jgarzik won't reveal. This is being reversed because Peter Todd and David Harding found a security vulnerability that would allow LN users to steal funds from each other. Notice how btc1 devs did not find it? Good luck with your fork.

This comment has been minimized.

Copy link

replied Oct 9, 2017

@CosmicHemorroid

they are the most familiar with the code

Innovtion gatekeeping that serves their roadmap of being critical and essential to bitcoin development (and you've fallen for it by the looks of it)

the best at what they do.

At being toxic, I agree.

The environment being fostered in the btc1 project will attract more developers to the new reference client link

This comment has been minimized.

Copy link

replied Oct 9, 2017

@ALL please keep it technical - we have enough toxicity in virtually every other channel that we don't need it in github as well.

This comment has been minimized.

Copy link

replied Oct 9, 2017

This comment has been minimized.

Copy link

replied Oct 10, 2017

Well @jgarzik this is a step in the right direction. I'd rather have no protection at all than that broken ass opt-in crap that people would be exploiting left and right. Thanx for the revert, glad you are finally seeing the light. I hope to see a true replay protection method soon. Who cares if wallets have to upgrade to support it like its some sort of altcoin. They will do so or the users of those wallets will complain and go elsewhere. It will just go to show that 2x can stand on its technical merits rather than hostile takeover attempts (well, that is IF you implement a replay protection thats worthwhile of course, I hope this commit is a step in that direction).

This comment has been minimized.

Copy link

replied Oct 11, 2017

Core should start considering to add in replay protection on their end, and possible POW change also? I know they have expressed desire to do so in the past, I think it would be courageous of them to stand up to these Chinese bully miners and fire them.

@zawy12

This comment has been minimized.

Copy link

commented Oct 9, 2017

@prusnak Miners maximizing profit in accordance with the rules is not malicious, but evidence of bad difficulty algorithm. The BCH difficulty algorithm should not have been asymmetric on rise v fall. It should have been changed to a simple rolling average with a much shorter window like most alt coins.

Since it looks like core will be the minority chain, maybe they need to HF to employ a much better difficulty algorithm like next_D=avg(past 30 D) * 600 / avg(past 30 Solvetimes) that will enable them to survive and let S2x miners/etc have the other chain. Since the majority chain minors could maliciously assign back-dated timestamps minority chain (6 of the past 11 to thwart MTP) to cause higher difficulty or negative or zero avg solvetime, a symmetrical limit on avg solvetmie is needed, such as > 0.7 * 600 and < 1/0.7 * 600.

@jgarzik

This comment has been minimized.

Copy link
Author

commented Oct 9, 2017

Rebased to latest segwit2x-dev.

Todo: Sunsetting

jgarzik and others added some commits Oct 10, 2017

Update README to link to segwit2x site.
Update a few other Bitcoin->btc1 Core references, to address complaints.
Explicitly skip broken tests so that the test runner doesn't complain…
… about them not being run at all.

Also reenabled walletbackup.py since it works locally.

This is step 1 towards re-enabling these tests properly.
Merge pull request #133 from christophebiocca/mark-broken-functional-…
…tests

Explicitly skip broken tests so that the test runner doesn't complain…
@jgarzik

This comment has been minimized.

Copy link
Author

commented Oct 12, 2017

Closing for procedural reasons - not based on correct branch - will open new PR, and reference this PR.

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.