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

November 2018 Upgrade #77

Open
HashEngineering opened this issue Aug 18, 2018 · 12 comments
Open

November 2018 Upgrade #77

HashEngineering opened this issue Aug 18, 2018 · 12 comments

Comments

@HashEngineering
Copy link
Collaborator

bitcoincashorg/bitcoincash.org#94

This pull request list coming proposed changes for Bitcoin Cash.

@rafa-js
Copy link

rafa-js commented Nov 3, 2018

What's going to happen with this?

@Danconnolly
Copy link
Collaborator

We will hopefully have someone working on this soon. Note that unless you are making non-standard transactions, the existing version should work correctly with either branch of Bitcoin Cash.

@rafa-js
Copy link

rafa-js commented Nov 5, 2018

That's nice! Do we need to include something in the DNS seed to support SV nodes?
Thanks @Danconnolly

@HashEngineering
Copy link
Collaborator Author

I had a bug in my app due to a larger block on Saturday (due to not updating to the May 2018 changes). This means that if the SV network creates a block > 32 MB, then bitcoinj-cash will either get stuck or follow the ABC change again.

There is a check for transaction count that is based on the max allowable block size. I can provide more details later.

@ilozanof
Copy link

Hi @HashEngineering

Can you provide more details of this issue? I'm working on same changes in the library and I'm planning to fix this as well. At the moment the lib works fine for me when connected to the blockchain network using the DNS discovery method. I'm in the middle of setting up a local SV node, so I can connect ONLY with it and verify if it works fine as well in that scenario.

The check about the message site (32 MB) is in place as you said, but I need to reproduce it before changing the source code. Most likely, I'll have to wait until my local SV node fully syncs with the blockchain, it will still take a couple of days.

@HashEngineering
Copy link
Collaborator Author

public static final int MAX_BLOCK_SIZE = 32 * 1000 * 1000;

This line needs to have the correct block size limit. While SPV nodes don’t download entire blocks, this value determines the maximum number of sigops and transactions allowed in a block.

@HashEngineering
Copy link
Collaborator Author

HashEngineering commented Dec 11, 2018

On November 19th there was a 38 mb and 64 mb block mined, which may cause bitcoinj to stop syncing depending on the number of transactions in those blocks.

@HashEngineering
Copy link
Collaborator Author

@ilozanof
Copy link

Thanks for all the info, @HashEngineering
an update on this issue:

Certainly, the MAX_BLOCK_SIZE should be updated. But that being said, it shouldn't make the biicoinj-cash to fail at this stage. That limit is mainly used in 2 scenarios: When verifying a Block (which is something that Bitcoinj-cash does not do, since the blocks downloaded are filtered blocks with only those Tx relevant to the Wallet), and when verifying an individual Transaction, which shouldn't fail unless the Tx is incredibly big (32MB in this case), which is not happening.

In my tests, I'm synchronizing to the SV chain and using in my wallet some keys which are referenced in some Tx contained in the block 557335, which is a 64MB block mined on November 20th. So far, no problems have popped up. You mentioned you had a bug in your app, but you also mentioned that the code was not updated with the last changes from May this year. Are you sure the problem is not caused by that, instead of the Block size limit?

The limit will be updated for sure (a PR will be submitted shortly), but the Lib should be safe and stable nevertheless.

PS: @rseibane, here you can find the changes needed in the DNS Seeds, to make sure you connect to the SV chain:
#83

Cheers

@rafa-js
Copy link

rafa-js commented Dec 17, 2018

Thank you @ilozanof, it's all done now 😊

@HashEngineering
Copy link
Collaborator Author

HashEngineering commented Dec 18, 2018

@ilozanof
This is the line I had this exception thrown VerificationException("Got a CPartialMerkleTree with more transactions than is possible")
https://github.com/bitcoinj-cash/bitcoinj/blob/cash-0.14/core/src/main/java/org/bitcoinj/core/PartialMerkleTree.java#L239

The value of my Block.MAX_BLOCK_SIZE was set at 8000000 and there was a block with more than 133333 transactions (https://blockchair.com/bitcoin-sv/block/556034) on November 10 (32MB). On September 4th there was a 23MB block which only had 97000 transactions so my app didn't throw the above exception.

With a 32MB block size, the max allowed transactions, based on that line, is: 533333. Block 557335 had only 334000 transactions so it caused no issues. https://blockchair.com/bitcoin-sv/block/557335

The limit of 533333 transactions will limit the allowed block size by bitcoinj-sv to 102MB (for transactions with only 1 input and 1 output, about 192 bytes) and 119MB (for transactions of 1 input and two outputs, about 225 bytes). Since many transactions involve more than 1 input (transactions > 300 bytes), that will mean that the block will fill up before the transaction limit is reached.

If a particular group is doing a stress test trying to generate as many small transactions as possible, then this particular limit of 533333 could be reached.

@ilozanof
Copy link

thanks for the detailed feedback, @HashEngineering

I can see now why I didn't get the exception. If you low the MAX_BLOCK_SIZE to another value, you might actually come across that problem. And as you said, with the current 32MB limit we are safe as long as we don't´get any block bigger than 120MB aprox (close to the theoretical limit of 128MB after the fork), and that is not very likely.

A new release of the lib will come shortly, including that change and maybe others as well, as @Danconnolly has stated in his notice here:
c3e90e2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants