Skip to content

Ports of Core's PR 7888, 8166, 8914#1004

Merged
gandrewstone merged 3 commits into
BitcoinUnlimited:devfrom
sickpig:ports_1
Mar 14, 2018
Merged

Ports of Core's PR 7888, 8166, 8914#1004
gandrewstone merged 3 commits into
BitcoinUnlimited:devfrom
sickpig:ports_1

Conversation

@sickpig

@sickpig sickpig commented Mar 12, 2018

Copy link
Copy Markdown
Collaborator

This a start to get our QA framework up-to-date importing enhancements made by Core on functional and unut tests frameworks.

Port Core's #8914: Kill insecure_random and associated global state
Port Core's #7888: prevector: fix 2 bugs in currently unreached code paths
Port Core's #8166: src/test: Do not shadow local variables

laanwj added 3 commits March 9, 2018 12:24
There are only a few uses of `insecure_random` outside the tests.
This PR replaces uses of insecure_random (and its accompanying global
state) in the core code with an FastRandomContext that is automatically
seeded on creation.

This is meant to be used for inner loops. The FastRandomContext
can be in the outer scope, or the class itself, then rand32() is used
inside the loop. Useful e.g. for pushing addresses in CNode or the fee
rounding, or randomization for coin selection.

As a context is created per purpose, thus it gets rid of
cross-thread unprotected shared usage of a single set of globals, this
should also get rid of the potential race conditions.

- I'd say TxMempool::check is not called enough to warrant using a special
  fast random context, this is switched to GetRand() (open for
  discussion...)

- The use of `insecure_rand` in ConnectThroughProxy has been replaced by
  an atomic integer counter. The only goal here is to have a different
  credentials pair for each connection to go on a different Tor circuit,
  it does not need to be random nor unpredictable.

- To avoid having a FastRandomContext on every CNode, the context is
  passed into PushAddress as appropriate.

There remains an insecure_random for test usage in `test_random.h`.
a7af72a prevector::swap: fix (unreached) data corruption (Kaz Wesley)
4ed41a2 test prevector::swap (Kaz Wesley)
1e2c29f prevector: destroy elements only via erase() (Kaz Wesley)
c2715d3 Do not shadow local variables (Pavel Janík)
@sickpig sickpig mentioned this pull request Mar 12, 2018
Comment thread src/prevector.h
{
item_ptr(size() - 1)->~T();
_size--;
erase(item_ptr(new_size), end());

@gandrewstone gandrewstone Mar 14, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't _size be set to new_size?
EDIT: nevermind, happens inside the erase() member function

@gandrewstone gandrewstone merged commit 05bc69c into BitcoinUnlimited:dev Mar 14, 2018
sickpig added a commit to sickpig/BitcoinUnlimited that referenced this pull request Jun 15, 2018
The latter definition was introduced by PR BitcoinUnlimited#1004, the former (SipHash)
since commit 77deb4e was introduced
(rel bucash 1.1.2.0)
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

Successfully merging this pull request may close these issues.

3 participants