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

fix issue where a block's coinbase can make it exceed the configured val #259

Merged
merged 4 commits into from Jan 31, 2017

Conversation

@gandrewstone
Copy link
Collaborator

commented Jan 29, 2017

fixes #258

…nvalid configuration left by a prior test
…ck generation with different length coinbase messages. Account for possible varint lengths of 9 bytes for 2 values that are not known during transaction selection

std::string testMinerComment("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLM");
// Now generate lots of full size blocks and verify that none exceed the maxGeneratedBlock value
//printf("test mining with different sized miner comments");

This comment has been minimized.

Copy link
@ptschip

ptschip Jan 30, 2017

Collaborator

@gandrewstone

  1. I think that by the time you get to running the second loop of tests there will be no more transactions in the pool since they would have been mined out by then, so you'll just be mining empty blocks. (2) I think you should put a step in there that modifies the miner string "after" the block is created and before we submit the block the way the miners do it...and then we submit the block and check that it was still accepted to and updates the blockchain.

This comment has been minimized.

Copy link
@ptschip

ptschip Jan 30, 2017

Collaborator

Actually on point number 1 I'm not right, these are just blocktemplates so the txns never get mined. So that looks fine.

This comment has been minimized.

Copy link
@gandrewstone

gandrewstone Jan 30, 2017

Author Collaborator

I delete the block rather than commit it. so the transactions are never removed from the mempool. This saves time since I don't have to make a bunch more transactions for every block size that is tested.

If you uncomment the printfs, you'll see the size of each block when it is created.

This comment has been minimized.

Copy link
@ptschip

ptschip Jan 30, 2017

Collaborator

ha, i just saw your comment, ignore my previous one...

std::string cbmsg = FormatCoinbaseMessage(BUComments, minerComment);
const char* cbcstr = cbmsg.c_str();
vector<unsigned char> vec(cbcstr, cbcstr+cbmsg.size());
COINBASE_FLAGS = CScript() << vec;

This comment has been minimized.

Copy link
@deadalnix

deadalnix Jan 30, 2017

Collaborator

This isn't thread safe. Also, capitalized names by convention are reserved for macros in C/C++ so that's confusing.

@ptschip

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2017

@deadalnix

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2017

i don't see where this is not thread safe...there a lock on cs_main as well as the memool?

Is it ? It is not taken in the function and there is no assertion that it is taken as precondition.

also, capital letter are used throughout bitcoin, there is nothing unusual or confusing about that IMO.

Everybody agree that the bitcoin codebase is not the highest quality standard you can find. Conclude.

@ptschip

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2017

@ftrader

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2017

The COINBASE_FLAGS capitalization might be due to it having been some constant earlier on in life. There is a misleading comment about it being constant where it is defined in main.cpp:123 - this should also be fixed as it's not constant now.

/** Constant stuff for coinbase transactions we create: */

@ptschip

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2017

other than the comment on main.cpp:123 that @ftrader highlighted, this PR looks good to me.

@gandrewstone

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 31, 2017

ok I'm going to merge with that comment because this is high priority and I'll fix the comment on the "dev" branch.

@gandrewstone gandrewstone merged commit 967b6d8 into BitcoinUnlimited:release Jan 31, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.