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

Bitcoin 0.12 test PRs 1 #2101

Merged
merged 6 commits into from Mar 3, 2017

Conversation

@str4d
Copy link
Contributor

commented Feb 14, 2017

Cherry-picked from the following upstream PRs:

Part of #2074.

@str4d str4d added this to the 1.0.7 milestone Feb 14, 2017
@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2017

d51ad27 needs careful checking against bitcoin/bitcoin@defd2d5 because we need to ensure that the improved test logic is actually maintained.

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2017

@zkbot try

zkbot added a commit that referenced this pull request Feb 15, 2017
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2017

⌛️ Trying commit a13ac3a with merge 57d420e...

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 15, 2017

💔 Test failed - zcash

@str4d str4d force-pushed the str4d:2074-tests branch from a13ac3a to e0bcc66 Feb 20, 2017
@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2017

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 20, 2017

⌛️ Trying commit e0bcc66 with merge 88c209d...

zkbot added a commit that referenced this pull request Feb 20, 2017
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 20, 2017

💔 Test failed - zcash

@str4d str4d force-pushed the str4d:2074-tests branch from e0bcc66 to 51c7823 Feb 20, 2017
@daira

This comment has been minimized.

@daira

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

Re: bitcoin/bitcoin#5881 , I can't confirm that this is only changing tests. The description says:

The second commit introduces a primitive method CTransaction::IsEquivalentTo and uses it in the wallet to ensure that SyncMetaData is only called for malleability clones, not general conflict (doublespend) transactions.

but there's no explanation of why this should be the case. (SyncMetaData is called by CWallet::AddToSpends which is called by CWallet::AddToWallet, which is not only called by tests; there's also some overloading of those methods which makes it difficult to follow the control flow.)

@daira

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

Re: bitcoin/bitcoin#6414 , I don't agree with moving the two tests to the extended test suite. Saving 15 seconds on the overall test time is not important.

@daira

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

Re: 0ffc9b1, I wasn't able to compile with clang in order to check what the results of compiling with -Wthread-safety are.

@radix42

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

oculdn't compile bitcoin or zcash with clang? because to my knowledge zcash hasn't built with clang since libsnark won't compile with a recent clang (at least since I've been trying)

@daira

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

For this specific purpose I don't need to compile the dependencies (or I can compile those with g++); I just need to see the output from attempting to compile the main zcash code with clang++. (It doesn't even need to link correctly.)

@radix42

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2017

oh I hadn't ever tried that on linux...failed when I tried that on Mac last fall....I'll have to give it a go sometime

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2017

@daira daira referenced this pull request Mar 2, 2017
@arcalinea

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2017

Keeping track of review and comments made so far.

PR @daira @bitcartel Will include (@arcalinea)
bitcoin/bitcoin#6337 ACK ACK Y
bitcoin/bitcoin#5881 NACK: can't confirm that this is only changing tests NACK Extracted to new PR
bitcoin/bitcoin@3f16971 NACK ACK Extracted to new PR
bitcoin/bitcoin#6365~ NACK ACK Extracted to new PR
bitcoin/bitcoin@56dc704 NACK ACK Extracted to new PR
bitcoin/bitcoin#6390 ACK ACK Y
bitcoin/bitcoin#6414 NACK, don't agree with moving the two tests to the extended test suite. Saving 15 seconds on the overall test time is not important ACK Extracted to new PR
bitcoin/bitcoin#5515 ACK ACK Y
bitcoin/bitcoin#6287 (partial, remainder included in bitcoin/bitcoin#6703) Couldn't test ACK Y
bitcoin/bitcoin#6465 ACK ACK Y

*Note: #5881 is NACK for now, but we should remember merge in older test changes before upgrading to Python 3 and changing bash test scripts to Python, to reduce merge conflict mess.

gavinandresen and others added 6 commits Jun 19, 2015
New, undocumented-on-purpose -mocktime=timestamp command-line
argument to startup with mocktime set. Needed because
time-related blockchain sanity checks are done on startup, before a
test has a chance to make a setmocktime RPC call.

And changed the setmocktime RPC call so calling it will not result in
currently connected peers being disconnected due to inactivity timeouts.
Fixes wrong scriptPubkey problem, which caused the transaction to
not actually be signed.
…acros

This allows us to use function/variable/class attributes to specify locking
requisites, allowing problems to be detected during static analysis.

This works perfectly with newer Clang versions (tested with 3.3-3.7). For older
versions (tested 3.2), it compiles fine but spews lots of false-positives.
This was chosen not because it's necessarily helpful, but because its locking
assumptions were already correct.
@str4d str4d force-pushed the str4d:2074-tests branch from 51c7823 to df8f809 Mar 3, 2017
@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2017

Rebased and trimmed PRs.

@arcalinea

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 3, 2017

📌 Commit df8f809 has been approved by arcalinea

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 3, 2017

⌛️ Testing commit df8f809 with merge dadb1ab...

zkbot added a commit that referenced this pull request Mar 3, 2017
Bitcoin 0.12 test PRs 1

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6337
- bitcoin/bitcoin#6390
- bitcoin/bitcoin#5515
- bitcoin/bitcoin#6287 (partial, remainder included in bitcoin/bitcoin#6703)
- bitcoin/bitcoin#6465

Part of #2074.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 3, 2017

☀️ Test successful - zcash

@zkbot zkbot merged commit df8f809 into zcash:master Mar 3, 2017
1 check passed
1 check passed
homu Test successful
Details
@str4d str4d referenced this pull request Mar 4, 2017
196 of 452 tasks complete
@str4d str4d deleted the str4d:2074-tests branch Mar 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.