{"action":"created","issue":{"url":"https://api.github.com/repos/hyperledger/fabric/issues/1798","repository_url":"https://api.github.com/repos/hyperledger/fabric","labels_url":"https://api.github.com/repos/hyperledger/fabric/issues/1798/labels{/name}","comments_url":"https://api.github.com/repos/hyperledger/fabric/issues/1798/comments","events_url":"https://api.github.com/repos/hyperledger/fabric/issues/1798/events","html_url":"https://github.com/hyperledger/fabric/pull/1798","id":159738997,"number":1798,"title":"No complaints preview PR","user":{"login":"jyellick","id":7431583,"avatar_url":"https://avatars.githubusercontent.com/u/7431583?v=3","gravatar_id":"","url":"https://api.github.com/users/jyellick","html_url":"https://github.com/jyellick","followers_url":"https://api.github.com/users/jyellick/followers","following_url":"https://api.github.com/users/jyellick/following{/other_user}","gists_url":"https://api.github.com/users/jyellick/gists{/gist_id}","starred_url":"https://api.github.com/users/jyellick/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jyellick/subscriptions","organizations_url":"https://api.github.com/users/jyellick/orgs","repos_url":"https://api.github.com/users/jyellick/repos","events_url":"https://api.github.com/users/jyellick/events{/privacy}","received_events_url":"https://api.github.com/users/jyellick/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":8,"created_at":"2016-06-10T23:00:45Z","updated_at":"2016-06-14T13:28:21Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/hyperledger/fabric/pulls/1798","html_url":"https://github.com/hyperledger/fabric/pull/1798","diff_url":"https://github.com/hyperledger/fabric/pull/1798.diff","patch_url":"https://github.com/hyperledger/fabric/pull/1798.patch"},"body":"## Description\r\nThis changeset is targeted at eliminating bugs discovered while working through the busywork stress2b test.  \r\n\r\nFirst, this changeset removes the old custody framework from batch, and replaces it with a much simpler broadcast and retain mechanism.\r\n\r\nSecondly, this changeset fixes several state transfer logic related bugs and consolidates this logic into one location in the code.\r\n\r\nThis PR is built on top of #1785, so please consider only the newer commits in your review for this PR.\r\n\r\n## Motivation and Context\r\nThe more sophisticated custody framework introduced which included deduplication, and selective broadcast of complaints is a technically superior solution, but delicate and difficult to implement.  There were several outstanding bugs, such as requests not being removed from custody when they should be, as well as the potential for some replicas to choose to include requests, and others to not, forking the blockchain.  In the future, hopefully the more robust complaints framework can be re-introduced, but until that time, this changeset considerably reduces the bug surface.\r\n\r\nThe state transfer logic was separated into multiple pieces of PBFT, and so changes to the logic required propagating it into all relevant pieces.  This was error prone and resulted in bugs, like not flagging state as invalid after a view change initiating state transfer, and increased the bug surface for conflicts like initiating state transfer during an execution.  Consolidating the logic makes for cleaner, and more visibly correct code.\r\n\r\nFixes #1091 \r\n\r\n## How Has This Been Tested?\r\nThis PR contains new unit tests for the naive custody logic, as well as new unit tests for the PBFT state transfer behavior.  This code has been tested to consistently pass the busywork `stress2b` test.\r\n\r\nThe failures discovered in the busywork tests were each converted into the unit tests mentioned above.\r\n\r\n## Checklist:\r\n- [X] I have added a [Signed-off-by](https://github.com/hyperledger/fabric/blob/master/CONTRIBUTING.md#legal-stuff)\r\n- [X] Either no new documentation is required by this change, OR I added new documentation\r\n- [X] Either no new tests are required by this change, OR I added new tests\r\n- [X] I have run [goimports](https://godoc.org/golang.org/x/tools/cmd/goimports), [go vet](https://golang.org/cmd/vet/), and [golint](https://github.com/golang/lint). I have cleaned up all valid errors and warnings in code I have added or modified. These tools may generate false positives. Don't be worried about ignoring some errors or warnings. The goal is clean, consistent, and readable code.\r\n\r\nSigned-off-by: Jason Yellick <jyellick@us.ibm.com>\r\n\r\n--------------------------------------\r\nThis PR was originally submitted as a preview, not ready for inclusion, below is the original text of this PR.\r\n\r\nThis pull request is intended as a preview for stability, especially for @bcbrock in regards to issue #1091.\r\n\r\nThere were several outstanding bugs in the logic of the complaints code, so this changeset removes it entirely from the logic flow of batch in favor of a much simpler broadcast and store outstanding policy.\r\n\r\nRunning the busywork stress2b test, things still don't complete successfully, but the block with no transaction bug seems to be gone, as does the 'null' block.  I'm hoping that @bcbrock can give some additional insight.\r\n\r\nI get errors like:\r\n\r\n```\r\ndriver:ERROR: Peer 0.0.0.0:34498 : Count mismatch for chaincode cc0, array a1.\r\nExpected 10000, Reported 9998 9997\r\ndriver:ERROR: Peer 0.0.0.0:53581 : Count mismatch for chaincode cc0, array a1.\r\nExpected 10000, Reported 340 7130\r\ndriver:ERROR: Aborting due to errors, mismatches or disagreements above\r\nmake: *** [stress2b] Error 1\r\n```\r\n\r\nwhich I think may be because executions and state transfer are still occurring.\r\n\r\nSome of the assorted bugs that were fixed via the view change state transfer need test cases added yet, as does the new naive broadcast and resubmission system, so this PR is not ready to be merged yet, but is intended to be visible for review, especially from @corecode @tuand27613 and @kchristidis.\r\n\r\nDCO signoff intentionally omitted for now."},"comment":{"url":"https://api.github.com/repos/hyperledger/fabric/issues/comments/225880479","html_url":"https://github.com/hyperledger/fabric/pull/1798#issuecomment-225880479","issue_url":"https://api.github.com/repos/hyperledger/fabric/issues/1798","id":225880479,"user":{"login":"srderson","id":4872087,"avatar_url":"https://avatars.githubusercontent.com/u/4872087?v=3","gravatar_id":"","url":"https://api.github.com/users/srderson","html_url":"https://github.com/srderson","followers_url":"https://api.github.com/users/srderson/followers","following_url":"https://api.github.com/users/srderson/following{/other_user}","gists_url":"https://api.github.com/users/srderson/gists{/gist_id}","starred_url":"https://api.github.com/users/srderson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/srderson/subscriptions","organizations_url":"https://api.github.com/users/srderson/orgs","repos_url":"https://api.github.com/users/srderson/repos","events_url":"https://api.github.com/users/srderson/events{/privacy}","received_events_url":"https://api.github.com/users/srderson/received_events","type":"User","site_admin":false},"created_at":"2016-06-14T13:28:21Z","updated_at":"2016-06-14T13:28:21Z","body":"@jyellick no problem, I can squash when I merge. Unfortunately it looks as though this needs to be rebased on master again, perhaps because I merged PR #1785? Also, I'm waiting for @kchristidis to complete his review of this PR."}}
{"actor":{"display_login":"srderson"}}