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

Migrate MiniNode to Zcash #2533

Merged
merged 11 commits into from Oct 16, 2017

Conversation

Projects
None yet
4 participants
@str4d
Contributor

str4d commented Jul 13, 2017

This enables various RPC tests that use it (most of them in the extended test suite) to properly test Zcash code.

The PR also fixes bugs in the BIP65 and BIP66 tests that were both masking and masked by the un-migrated MiniNode.

The Python module pyblake2 is now a requirement for the RPC tests.

Part of #2530.

@str4d str4d added the testing label Jul 13, 2017

str4d added a commit to str4d/zcash that referenced this pull request Jul 13, 2017

str4d added a commit to str4d/zcash that referenced this pull request Jul 14, 2017

@nathan-at-least nathan-at-least added this to Proposed in Release planning Aug 15, 2017

@nathan-at-least nathan-at-least moved this from Proposed to Test Improvements in Release planning Aug 15, 2017

@str4d str4d self-assigned this Aug 15, 2017

@str4d str4d added this to the 1.0.13 milestone Oct 2, 2017

@str4d str4d requested review from bitcartel and daira Oct 2, 2017

Add connections in BIP65 and BIP66 tests to the test manager
Fixes a bug in the tests causing them to silently pass instead of correctly
reporting other errors. Introduced in 4a785b0
during the test rewrites.
@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 3, 2017

Contributor

Rebased on master, and inserted a commit that exposes a bug in the RPC tests which this PR fixes (uncovering other as-yet unfixed bugs). Note that GitHub doesn't show the commit ordering correctly; I inserted the commit so that in the git history, the RPC tests would break, and then the MiniNode commits would fix them.

Contributor

str4d commented Oct 3, 2017

Rebased on master, and inserted a commit that exposes a bug in the RPC tests which this PR fixes (uncovering other as-yet unfixed bugs). Note that GitHub doesn't show the commit ordering correctly; I inserted the commit so that in the git history, the RPC tests would break, and then the MiniNode commits would fix them.

@str4d str4d modified the milestones: 1.0.13, 1.0.13 CI release Oct 4, 2017

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 4, 2017

Contributor

Moving to CI milestone because updates to rpc-tests.sh are blocking on this.

Contributor

str4d commented Oct 4, 2017

Moving to CI milestone because updates to rpc-tests.sh are blocking on this.

@daira

daira approved these changes Oct 4, 2017

utACK, no comments.

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Oct 4, 2017

Contributor

@zkbot try

Contributor

bitcartel commented Oct 4, 2017

@zkbot try

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 4, 2017

Contributor

⌛️ Trying commit 14f28c4 with merge 9fd14ca...

Contributor

zkbot commented Oct 4, 2017

⌛️ Trying commit 14f28c4 with merge 9fd14ca...

zkbot added a commit that referenced this pull request Oct 4, 2017

Auto merge of #2533 - str4d:2530-mininode, r=<try>
Migrate MiniNode to Zcash

This enables various RPC tests that use it (most of them in the extended test suite) to properly test Zcash code.

The PR also fixes bugs in the BIP65 and BIP66 tests that were both masking and masked by the un-migrated MiniNode.

The Python module `pyblake2` is now a requirement for the RPC tests.

Part of #2530.
@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Oct 4, 2017

Contributor

Running a zkbot try. Locally, I'm finding the z_sendmany tests are hanging. Maybe other tests too?

Contributor

bitcartel commented Oct 4, 2017

Running a zkbot try. Locally, I'm finding the z_sendmany tests are hanging. Maybe other tests too?

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 4, 2017

Contributor

💔 Test failed - pr-try

Contributor

zkbot commented Oct 4, 2017

💔 Test failed - pr-try

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 4, 2017

Contributor

Failed because of pyflakes errors. Otherwise, the supported (Debian 8) builders all passed.

However, the unsupported builders all hung in ways they didn't previously:

  • ubuntu16-04 hung in wallet_shieldcoinbase.py (normally passes)
  • debian9 hung in prioritisetransaction.py at start of RPC tests (normally hangs in wallet_protectcoinbase.py with an apparent crash of the node)
  • centos7 hung in wallet_shieldcoinbase.py (normally passes the RPC tests)
  • archlinux hung in prioritisetransaction.py (normally passes the RPC tests)

We don't need to investigate these, but we probably should.

Contributor

str4d commented Oct 4, 2017

Failed because of pyflakes errors. Otherwise, the supported (Debian 8) builders all passed.

However, the unsupported builders all hung in ways they didn't previously:

  • ubuntu16-04 hung in wallet_shieldcoinbase.py (normally passes)
  • debian9 hung in prioritisetransaction.py at start of RPC tests (normally hangs in wallet_protectcoinbase.py with an apparent crash of the node)
  • centos7 hung in wallet_shieldcoinbase.py (normally passes the RPC tests)
  • archlinux hung in prioritisetransaction.py (normally passes the RPC tests)

We don't need to investigate these, but we probably should.

str4d added some commits Jul 13, 2017

[Test] MiniNode: Use Zcash PoW
Equihash solver code extracted from https://github.com/str4d/zcash-pow

RPC tests now require pyblake2 to be installed
[Test] MiniNode: Fix coinbase creation
CScriptNum is only used for heights > 16.
[Test] MiniNode: Coerce OP_PUSHDATA bytearrays to bytes
If a bytearray is passed in as part of an iterable, the CScript constructor
fails because b''.join() cannot be used to join a bytearray to a bytes or str in
Python 2.

str4d added some commits Oct 4, 2017

Fix BIP65 and BIP66 tests
Blocks were being created that didn't satisfy the regtest consensus rules.
Un-indent RPC test output in test runner
The indentation caused the test stdout to be buffered and only printed at the
end of the test, which makes it harder to diagnose hanging tests.
@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 5, 2017

Contributor

Fixed the pyflakes errors, and tweaked the RPC test runner to make diagnosing the hanging tests easier.

@zkbot try

Contributor

str4d commented Oct 5, 2017

Fixed the pyflakes errors, and tweaked the RPC test runner to make diagnosing the hanging tests easier.

@zkbot try

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 5, 2017

Contributor

⌛️ Trying commit f8ef223 with merge 6c8687a...

Contributor

zkbot commented Oct 5, 2017

⌛️ Trying commit f8ef223 with merge 6c8687a...

zkbot added a commit that referenced this pull request Oct 5, 2017

Auto merge of #2533 - str4d:2530-mininode, r=<try>
Migrate MiniNode to Zcash

This enables various RPC tests that use it (most of them in the extended test suite) to properly test Zcash code.

The PR also fixes bugs in the BIP65 and BIP66 tests that were both masking and masked by the un-migrated MiniNode.

The Python module `pyblake2` is now a requirement for the RPC tests.

Part of #2530.
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 5, 2017

Contributor

☀️ Test successful - pr-try
State: approved= try=True

Contributor

zkbot commented Oct 5, 2017

☀️ Test successful - pr-try
State: approved= try=True

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 5, 2017

Contributor

The builders which normally pass RPC tests but were hanging are now passing again; possibly an intermittent failure. More interestingly, the Debian 9 builder failed as it usually does in wallet_protectcoinbase.py, but continued on to the end of the RPC tests (also failing in wallet.py), finished them, and then hung there. I'll keep this in mind when testing the improvements to the RPC test runner that I'm pulling in from upstream, but as far as this PR goes, I'm happy as-is.

Contributor

str4d commented Oct 5, 2017

The builders which normally pass RPC tests but were hanging are now passing again; possibly an intermittent failure. More interestingly, the Debian 9 builder failed as it usually does in wallet_protectcoinbase.py, but continued on to the end of the RPC tests (also failing in wallet.py), finished them, and then hung there. I'll keep this in mind when testing the improvements to the RPC test runner that I'm pulling in from upstream, but as far as this PR goes, I'm happy as-is.

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Oct 12, 2017

Contributor

Looks like the Debian 9 builder is failing, like Ubuntu locally on wallet_protectcoinbase.py and wallet.py. Is the problem on Debian 9 also a "broken pipe error"? Add Debian 9 to this ticket #2263 ?

I also had 'walletbackup.py' fail, but running that on its own passed.

Contributor

bitcartel commented Oct 12, 2017

Looks like the Debian 9 builder is failing, like Ubuntu locally on wallet_protectcoinbase.py and wallet.py. Is the problem on Debian 9 also a "broken pipe error"? Add Debian 9 to this ticket #2263 ?

I also had 'walletbackup.py' fail, but running that on its own passed.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 12, 2017

Contributor

Is the problem on Debian 9 also a "broken pipe error"?

That's not the error we've seen on CI before (we get httplib.CannotSendRequest). However, the Debian 9 builder failure in the latest test of this PR is the same as we see on other PRs, indicating no negative change due to this PR.

The Debian 9 builder is still an unsupported platform (as is Ubuntu, although those tests generally pass on the CI), and therefore is not a blocker for merging.

Contributor

str4d commented Oct 12, 2017

Is the problem on Debian 9 also a "broken pipe error"?

That's not the error we've seen on CI before (we get httplib.CannotSendRequest). However, the Debian 9 builder failure in the latest test of this PR is the same as we see on other PRs, indicating no negative change due to this PR.

The Debian 9 builder is still an unsupported platform (as is Ubuntu, although those tests generally pass on the CI), and therefore is not a blocker for merging.

@bitcartel

This comment has been minimized.

Show comment
Hide comment
@bitcartel

bitcartel Oct 12, 2017

Contributor

Yes, it shows as httplib.CannotSendRequest and if you scroll up in the console output, you should see (at least on Ubuntu) that the underlying cause is a broken pipe error. Do you see it on Debian 9 output?

Note that #2263 only started happening once we merged the backports related to HTTP event, so perhaps we missed something along the way.

Contributor

bitcartel commented Oct 12, 2017

Yes, it shows as httplib.CannotSendRequest and if you scroll up in the console output, you should see (at least on Ubuntu) that the underlying cause is a broken pipe error. Do you see it on Debian 9 output?

Note that #2263 only started happening once we merged the backports related to HTTP event, so perhaps we missed something along the way.

@str4d

This comment has been minimized.

Show comment
Hide comment
@str4d

str4d Oct 16, 2017

Contributor

@zkbot r+

Contributor

str4d commented Oct 16, 2017

@zkbot r+

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 16, 2017

Contributor

📌 Commit f8ef223 has been approved by str4d

Contributor

zkbot commented Oct 16, 2017

📌 Commit f8ef223 has been approved by str4d

@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 16, 2017

Contributor

⌛️ Testing commit f8ef223 with merge a2dc9be...

Contributor

zkbot commented Oct 16, 2017

⌛️ Testing commit f8ef223 with merge a2dc9be...

zkbot added a commit that referenced this pull request Oct 16, 2017

Auto merge of #2533 - str4d:2530-mininode, r=str4d
Migrate MiniNode to Zcash

This enables various RPC tests that use it (most of them in the extended test suite) to properly test Zcash code.

The PR also fixes bugs in the BIP65 and BIP66 tests that were both masking and masked by the un-migrated MiniNode.

The Python module `pyblake2` is now a requirement for the RPC tests.

Part of #2530.
@zkbot

This comment has been minimized.

Show comment
Hide comment
@zkbot

zkbot Oct 16, 2017

Contributor

☀️ Test successful - pr-merge
Approved by: str4d
Pushing a2dc9be to master...

Contributor

zkbot commented Oct 16, 2017

☀️ Test successful - pr-merge
Approved by: str4d
Pushing a2dc9be to master...

@zkbot zkbot merged commit f8ef223 into zcash:master Oct 16, 2017

1 check passed

homu Test successful
Details

@str4d str4d deleted the str4d:2530-mininode branch Oct 16, 2017

@daira daira added this to Complete in Security and Stability Nov 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment