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

[hardfork] Make sure P2Pool is compliant with SegWit #327

Merged
merged 10 commits into from Aug 23, 2017

Conversation

Projects
None yet
@veqtrus
Copy link
Contributor

veqtrus commented Nov 13, 2016

Transactions are now identified by their hashes, txids and wtxids. Internally the hash is used for tracking transactions so that an attacker cannot mislead a node into accepting a transaction which is functionally the same with another one but results in a different witness_commitment_hash. The others are used only for merkle root commitments.

The new share format is incompatible with older nodes since new fields are added to share_info and an additional output is added to gentx.

Please help testing by mining on the Testnet.

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Nov 18, 2016

I will be keeping only a U3 mining on the testnet so if anyone wants to join testing segwit let me know to add more hashpower.

@RubyRod

This comment has been minimized.

Copy link

RubyRod commented Nov 18, 2016

I have some old BFL SC 60GH/s I can point there if you need them.

Cheers,
Jean

On Nov 18, 2016, at 8:51 AM, Paul Georgiou notifications@github.com wrote:

I will be keeping only a U3 mining on the testnet so if anyone wants to join testing segwit let me know to add more hashpower.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Nov 18, 2016

@RubyRod I do own better hardware but I would need more peers to mine. So if you can run my PR code in testnet mode and point some hashpower at it, it would be helpful. Make sure to first download the sharechain from my node.

@veqtrus veqtrus force-pushed the veqtrus:segwit1 branch from f954f33 to 1a30d94 Nov 23, 2016

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Nov 23, 2016

Fixed segwit_data packing and block weight calculation. b0f9902

Edit: Changed how segwit_data == None is packed. Since wtxid_merkle_root can be 0 if only the gentx is present the value is now 2**256-1. 40506a1

@veqtrus veqtrus force-pushed the veqtrus:segwit1 branch from 1a30d94 to b732829 Nov 23, 2016

@veqtrus veqtrus force-pushed the veqtrus:segwit1 branch from b732829 to cc59bc2 Dec 9, 2016

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Dec 9, 2016

P2Pool will produce blocks with witnesses upon activation on the P2Pool network, which can happen before activation on the Bitcoin network. Bitcoin Core doesn't accept such blocks before activation (even though it could strip the witnesses) so I made it so appropriate block representations will be submitted through the RPC interface. b7f7a3c

Please note that submitting via the Bitcoin P2P network doesn't work anyway since headers-first synchronisation (P2Pool doesn't support the getheaders message).

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Dec 19, 2016

@luke-jr could you review this?

@luke-jr

This comment has been minimized.

Copy link

luke-jr commented Dec 19, 2016

Afraid I've always found p2pool code to be unreviewable.

I don't see where it fails if unrecognised '!' rules are specified, however?

@veqtrus veqtrus force-pushed the veqtrus:segwit1 branch 2 times, most recently from 72c6100 to f7fe251 Dec 23, 2016

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Dec 23, 2016

Updated as per @luke-jr's suggestion.

@tecnomexico

This comment has been minimized.

Copy link

tecnomexico commented Dec 23, 2016

Good morning

I am installing a wallet bitcoincore
When you open 2 clients

Original and testnet
It is normal.

it is synchronizing.

I can help with something

Happy Holidays

@veqtrus veqtrus force-pushed the veqtrus:segwit1 branch from f7fe251 to daef56b Dec 25, 2016

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Dec 25, 2016

Ping @theuni @jameshilliard

Could you take a look at this?

@theuni

This comment has been minimized.

Copy link

theuni commented Dec 29, 2016

@veqtrus Sorry, but I'm not capable of usefully reviewing this either.

@p3yot3

This comment has been minimized.

Copy link

p3yot3 commented Jan 30, 2017

Is this working? If so, when will it be merged?

Thanks.

@forrestv

This comment has been minimized.

Copy link
Member

forrestv commented Jan 30, 2017

@ChainQuery

This comment has been minimized.

Copy link
Contributor

ChainQuery commented Jan 30, 2017

@forrestv There is no projected activation date, currently at about 24% of the 95% required for activation. Source: https://bitcoincore.org/en/segwit_adoption/#miner-signalling

@p3yot3

This comment has been minimized.

Copy link

p3yot3 commented Jan 30, 2017

I was just wondering, as there are some altcoins that are also implementing segwit which have a much smaller userbase/hashrate, meaning adoption would be much faster, so if this patch is working could it also be applied to the various altcoin p2pool forks?

Thanks.

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Jan 31, 2017

@p3yot3 You can check blocks mined to mz5ydTb59nk3AMPrpVtDQi2WGy7F26b8aY (there are others but this is my address). Stats at http://veqtrus.eu:19332/static/ (you will be automatically connected to my node if you run my PR).

@veqtrus veqtrus force-pushed the veqtrus:segwit1 branch 2 times, most recently from 4759554 to d8f6d1b Feb 14, 2017

@veqtrus veqtrus force-pushed the veqtrus:segwit1 branch from d8f6d1b to d4543bd Feb 20, 2017

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Feb 28, 2017

To allow p2pool to produce larger blocks in the future I increased the new txns per share size limit from 50 to 100 kB, activated along with segwit.

@jtoomim

This comment has been minimized.

Copy link

jtoomim commented Apr 3, 2017

As discussed in 341, the current share size limit of 50 kB is much too small, so the proposed SegWit share size limit of 100 kB will also be too small.

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Apr 3, 2017

As discussed during the bitcoin scaling debate 1 MB per 30 seconds is way too much. 100 kB per share is enough for most blocks to be big.

@jtoomim

This comment has been minimized.

Copy link

jtoomim commented Apr 3, 2017

@veqtrus I think the share size discussion is better held in #341, which is focused on the topic. I will reply in that thread.

])
def handle_reject(self, message, ccode, reason, data):
if p2pool.DEBUG:
print >>sys.stderr, 'Received reject message (%s): %s' % (message, reason)

This comment has been minimized.

@jtoomim

jtoomim Jul 19, 2017

Are we sure that we only want to print this message if we're running in debug mode? Would this be an infrequent error message which should never occur unless something is seriously wrong?

This comment has been minimized.

@veqtrus

veqtrus Jul 20, 2017

Author Contributor

I've only used it when debugging the witness commitment calculation. You may be right though.

@btcdrak

This comment has been minimized.

Copy link

btcdrak commented Jul 20, 2017

Probably want to get on with this patch because segwit seems like it will be activated within the next 5 weeks.

@jtoomim

This comment has been minimized.

Copy link

jtoomim commented Jul 20, 2017

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Jul 20, 2017

Before this gets merged I would like to consider increasing the new tx size limit to 250 kB to avoid the two networks situation. Also it would be good to cherry pick jtoomim's performace improvements.

@jtoomim would this be an acceptable compromise?

@jtoomim

This comment has been minimized.

Copy link

jtoomim commented Jul 20, 2017

I personally am not interested in running a version of p2pool that would prevent my node from consistently creating blocks at 999 kB (or equivalent block weight). 250 kB per share would limit around 5% of blocks to 250 kB, 5% to 500 kB, and 5% to 750 kB. That's an average blocksize reduction of 5%. I consider that unacceptable. After SegWit is enabled, assuming 1.7 MB per SegWit block typical, we'd get around 35% of blocks limited to less than their potential size. After Segwit2x is enabled, assuming 3.4 MB per block (and assuming the mempool is full, which is unlikely), we'd get as much as 70% of blocks limited to less than their potential size.

I think the better approach to satisfying your concerns is just to strip all of the transaction data out of the shares and remove/disable p2pool's fast block propagation code. That will solve the bandwidth, CPU, and memory requirement issues that p2pool has entirely, and make the question of how many new transactions per block moot. P2pool's fast block propagation code was great when it was first introduced, but it's now obsolete (about 10x slower than Bitcoin FIBRE) and holding the project back.

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Aug 13, 2017

@forrestv SegWit has locked-in and is expected to activate around 2017-08-22. It's time to merge this PR.

@jtoomim

This comment has been minimized.

Copy link

jtoomim commented Aug 14, 2017

The current version of this code is incompatible with SegWit2x, as it lacks segwit2x in the REQUIRED_SOFTFORKS variable for the bitcoin network, and as veqtrus's code is designed to not mine if the bitcoind process reports a required softfork that is not in p2pool's REQUIRED_SOFTFORKS variable. As such, attempts to mine with btc1 will fail.

Given that SegWit2x has approximately 92% hashrate support at the moment, and that the only reason why SegWit is being activated is due to support for SegWit2x among miners, I consider the incompatibility of this PR with SegWit2x to be a bug.

P2pool should not force users to follow a minority branch in a hard fork. It should either give users the option of which branch to follow, or it should follow the majority branch.

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Aug 14, 2017

Re 2x:

This is by design as this PR targets mainly Bitcoin and not the 2x altcoin. People who wish to mine 2x should fork and add the appropriate network config files. The main P2Pool can't support all the altcoins out there.

100% of miners are irrelevant to what Bitcoin is so this code follows the (economic) majority branch. SegWit is being activated because > 95% of miners signaled for it and because the economic majority supports it as seen here. While a significant portion of miners signals for 2x this is irrelevant as 2x blocks are incompatible with Bitcoin and therefore will likely turn out to be false signaling once miners realize that they are mining fool's gold.

@midnightmagic

This comment has been minimized.

Copy link
Contributor

midnightmagic commented Aug 15, 2017

P2Pool has, except for the forks for altcoin support, followed canonical core consensus. Incompatible consensus splits from that should take care to ensure they don't interfere with the primary P2Pool network, IRC channel (for announces,) peer discovery, and so on, as per e.g. some of the altcoin support branches.

Strongly recommend that any s2x support be carefully split from the main P2Pool consensus network by functionality.

@jtoomim

This comment has been minimized.

Copy link

jtoomim commented Aug 15, 2017

P2Pool has, except for the forks for altcoin support, followed canonical core consensus

That's technically true, but it's equally true that P2Pool has always followed the hashrate majority. Bitcoin Core has never disagreed with the hashrate majority before this instance. I don't see how trying to cite precedent can be helpful when no precedent exists.

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Aug 15, 2017

@jtoomim brought up that p2pool may not recognise that it is mining on top of an invalid block because of its headers-only initial work. Tentative partial fix for bug #305 (#334) switches to the header reported by bitcoind after 3 attempts to check that the header received from the last share is the best.

@luke-jr

This comment has been minimized.

Copy link

luke-jr commented Aug 15, 2017

2X is an altcoin, nothing more. Segwit was activated by the community deploying BIP148.

@veqtrus Note that mining software will not allow switching back to past prevblocks. So you can't go from A to B then back to A.

@veqtrus veqtrus force-pushed the veqtrus:segwit1 branch from 5f63bad to 4d5c201 Aug 15, 2017

@veqtrus veqtrus force-pushed the veqtrus:segwit1 branch from 4d5c201 to 519486f Aug 15, 2017

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Aug 15, 2017

Instead of ugly hacks p2pool will simply not accept unknown headers from peers. 519486f

@forrestv forrestv merged commit 62fa7b0 into p2pool:master Aug 23, 2017

@jtoomim

This comment has been minimized.

Copy link

jtoomim commented Aug 23, 2017

@forrestv It looks like this is too late to activate before Segwit is active on mainnet. https://bitcointalk.org/index.php?topic=18313.msg21141211#msg21141211

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Aug 23, 2017

@forrestv #351 for immediate activation of segwit logic.

@veqtrus

This comment has been minimized.

Copy link
Contributor Author

veqtrus commented Aug 23, 2017

@forrestv Since v17 shares will not activate on time to ensure compatibility see #352

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment