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

add lock for nLargestBlockSeen #180

Merged
merged 1 commit into from Dec 12, 2016

Conversation

Projects
None yet
8 participants
@ptschip
Copy link
Collaborator

commented Dec 10, 2016

No description provided.

src/unlimited.cpp Outdated
uint64_t LargestBlockSeen(uint64_t nBlockSize)
{
// return the largest block size that we have seen since startup
static uint64_t nLargestBlockSeen = BLOCKSTREAM_CORE_MAX_BLOCK_SIZE;

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Dec 10, 2016

Contributor

Can you please keep your code base free of untruthful and defamatory misuse of my companies trademarks?

Edit: Oh god, rbtc is being a typical bunch of fools. The above is no way a legal threat, but a horrified complaint about the lack of professionalism and basic decency exhibited here; especially towards people whos work and charity this project has been highly dependent on.

This comment has been minimized.

Copy link
@deadalnix

deadalnix Dec 10, 2016

Collaborator

This is the block size which is preferred by Blockstream and Core at the moment. It is neither unthruthful nor defamatory, simply accurate.

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Dec 10, 2016

Contributor

Blockstream has no "preferred blocksize" and "blockstream core" isn't something that exists, except as a really nasty insult by a collection of dishonest people who are incorrectly claiming that the work of the Bitcoin project's is somehow owned by blockstream because a couple blockstream employees contributed to it. Considering that we've also written pretty much just as much of Bitcoin Unlimited, you might as well call it "Blockstream Unlimited".

This comment has been minimized.

Copy link
@painlord2k

painlord2k Dec 10, 2016

We don't claim the work on Core is "owned" by Blockstream.
We believe many Core's developers are owners, owned or hired by Blockstream. And this gives Blockstream (AKA you) the ability to hijack the development of Core in a direction favorable only to Blockstream's interests.
We believe we are entitled to our beliefs.
We believe we are entitled to our freedom of speech and writing, as you are.

Would you prefer "BULLSHIT_CO_MAX_BLOCK_SIZE"?
This would refer more to your attitude than your company.

BTW, I don't believe the use of "BLOCKSTREAM_CORE_MAX_BLOCK_SIZE" as a name of a variable inside a GitHub repository infringe any of your rights.

This comment has been minimized.

Copy link
@fraggle222

fraggle222 Dec 10, 2016

gmaxwell is correct (but wrong too). Terms like "BLOCKSTREAM_CORE..." inside the codebase are just inserting political bullshit into your code. It should obviously be changed to something more sensible. Leaving it there just makes BU look silly.

He is wrong though in claiming (in his edit) that in no way a legal threat was made. Using terms like trademark and defamatory imply a legal basis for pursuing action (especially when done by an officer of a company), and implied legal action is a form of legal threat ("we could sue you" is not "we will sue you if", but still can be threatening).

Anyway the variable name should be changed, regardless of any trademark issues.

This comment has been minimized.

Copy link
@ftrader

ftrader Dec 10, 2016

Collaborator

OVERLY_RESTRICTIVE_CORE_MAX_BLOCK_SIZE ?

This comment has been minimized.

Copy link
@illuzen

illuzen Dec 12, 2016

Did you just start reading the BU code, @gmaxwell ?

@deadalnix

This comment has been minimized.

Copy link
Collaborator

commented Dec 10, 2016

Using atomics sounds more appropriate here. acquire when reading, release when writing.

@ptschip

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 10, 2016

@deadalnix I had a feeling you would say that...i originally wrote it with atomic but changed it because the code looked ugly because of the initialization. This function doesn't get called that frequently so it's not that big a deal IMO, But ah, ok, i'll put another commit with atomic on top of this one before squashing and everyone can comment on which one looks best.

@deadalnix

This comment has been minimized.

Copy link
Collaborator

commented Dec 10, 2016

Initialization is made by the system before running you program so there is no reason to do it differently than what is done here.

@ptschip

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 10, 2016

@deadalnix @gandrewstone ok you can take a look, both commits are still there. I think it's a pretty small optimization for this atomic but faster no doubt.

src/unlimited.cpp Outdated
static boost::atomic<uint64_t> nLargestBlockSeen;
static bool fInit = true;
if (fInit) {
nLargestBlockSeen.store(BLOCKSTREAM_CORE_MAX_BLOCK_SIZE);

This comment has been minimized.

Copy link
@deadalnix

deadalnix Dec 10, 2016

Collaborator

No just keep the old initialization. it'll contains the value even BEFORE the program starts as it'll be in the data segment.

This comment has been minimized.

Copy link
@ptschip

ptschip Dec 10, 2016

Author Collaborator

@deadalnix do you mean this:

static uint64_t nLargestBlockSeen = BLOCKSTREAM_CORE_MAX_BLOCK_SIZE;
static boost::atomic<uint64_t> nLargestBlockSeen;

because this would/does not work...so I'm not sure what you're referring to.

@deadalnix

This comment has been minimized.

Copy link
Collaborator

commented Dec 10, 2016

Use initializer.

static std::atomic<uint64_t> nLargestBlockSeen{BLOCKSTREAM_CORE_MAX_BLOCK_SIZE};
@ptschip

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 10, 2016

@deadalnix that method does not work in C++98 ... we should be on C++11 soon i hope.

@ptschip ptschip force-pushed the ptschip:12.1nlargest branch Dec 10, 2016

@ptschip

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 10, 2016

@deadalnix Once we get to c++11 we can cleanup the init. I'm going to squash the previous commit using the recursive mutex and keep this one only.

@deadalnix

This comment has been minimized.

Copy link
Collaborator

commented Dec 10, 2016

So, I think we should move to C++11 . Core did it. The previous standard is 20 years old and do not provide adequate means to deals with multicore.

But if you don't want that (after all, that increase the scope of this diff quite a bit) then I think you can add a runtime check for value smaller than BLOCKSTREAM_CORE_MAX_BLOCK_SIZE and act accordingly.

src/unlimited.cpp Outdated
if (fInit) {
nLargestBlockSeen.store(BLOCKSTREAM_CORE_MAX_BLOCK_SIZE);
fInit = false;
}

This comment has been minimized.

Copy link
@deadalnix

deadalnix Dec 10, 2016

Collaborator

This isn't thread safe. There is no ordering constraint on the flag, so it can be done in any order. You may end up seeing the variable uninitialized and the flag sset in another thread.

This comment has been minimized.

Copy link
@ptschip

ptschip Dec 11, 2016

Author Collaborator

I got the init to work using () instead of {}...thanks to Tom Zander

src/unlimited.cpp Outdated

uint64_t nSize = nLargestBlockSeen.load();
if (nBlockSize > nSize) {
nLargestBlockSeen.store(nBlockSize);

This comment has been minimized.

Copy link
@deadalnix

deadalnix Dec 10, 2016

Collaborator

You store based on a choice you made on the previous value. there fore, you need to do compare and swap in a loop.

This comment has been minimized.

Copy link
@ptschip

ptschip Dec 11, 2016

Author Collaborator

this works just as it should...we're just swapping the old with the new if the blocksize has increased...not need for a loop here.

This comment has been minimized.

Copy link
@deadalnix

deadalnix Dec 12, 2016

Collaborator

The value may have changed between the load and the store. You need to use CAS.

@ptschip ptschip force-pushed the ptschip:12.1nlargest branch 7 times, most recently Dec 11, 2016

Peter Tschipper

@ptschip ptschip force-pushed the ptschip:12.1nlargest branch to fe569d5 Dec 12, 2016

@gandrewstone gandrewstone merged commit 8316d84 into BitcoinUnlimited:0.12.1bu Dec 12, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ptschip ptschip deleted the ptschip:12.1nlargest branch Dec 12, 2016

// eturn the largest block size that we have seen since startup
uint64_t nSize = nLargestBlockSeen.load();
if (nBlockSize > nSize) {
nLargestBlockSeen.store(nBlockSize);

This comment has been minimized.

Copy link
@deadalnix

deadalnix Dec 12, 2016

Collaborator

Guys, seriously, this is invalid. I need to use CAS and that was mentioned already.

@@ -38,6 +39,7 @@
using namespace std;

extern CTxMemPool mempool; // from main.cpp
static boost::atomic<uint64_t> nLargestBlockSeen(BLOCKSTREAM_CORE_MAX_BLOCK_SIZE); // track the largest block we've seen

This comment has been minimized.

Copy link
@deadalnix

deadalnix Dec 12, 2016

Collaborator

This is a pre main runtime initialization. You may see 0 in there.

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.