-
Notifications
You must be signed in to change notification settings - Fork 29
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
ECR-1624 Messages payload length fix #285
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 , Thanks!
I also propose some little improvements in the exception messages in #287
@@ -13,7 +13,7 @@ | |||
int VERSION_OFFSET = 1; | |||
int MESSAGE_TYPE_OFFSET = 2; | |||
int SERVICE_ID_OFFSET = 4; | |||
int BODY_LENGTH_OFFSET = 6; | |||
int PAYLOAD_LENGTH_OFFSET = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little misleading name in the specification is one of the causes of this bug, I propose to use something more specific: MESSAGE_SIZE_OFFSET
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? I agree that this naming is ambiguous, but on the other hand it is consistent with our documentation - https://exonum.com/doc/architecture/serialization/#message-serialization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way, there will be surprises/inconsistencies. OK, let's keep it as is, but request the core to use clearer names:
https://jira.bf.local/browse/ECR-1647
- message serialization is expected to change, @MakarovS , would you review this document please: https://wiki.bf.local/pages/viewpage.action?pageId=10094093
- Clarify the exception messages - Make size() return the value from the header (also check it matches the buffer size). - Add more tests - Extract allocateMessageBuffer
* Add third party licenses for direct Rust dependencies [ECR-1502]. (#277) * Update the project dependencies [ECR-1521, ECR-1518]: - Add versions plugin - Update the plugins that we use, specify the versions in the pluginManagement. - Update the versions of dependencies. - Disable Powermock tests until the library is updated. - Verify the project builds on 10, run tests with openjdk-10 on the CI server. - Check for updates on the CI server. * Use the default Java compiler instead of error-prone: Error-prone does not support Java 10 yet. A setup to activate it conditionally would be too verbose (a profile activation in each sub-module, because it does not work for <build><pluginManagement>...). See google/error-prone#860 * Do not install RocksDb — prevents SIGILL on older systems. On some systems with older CPUs our build produces an executable with an illegal instruction for those processors. The problem needs further investigation, but if we do not use the system version of RocksDb, it goes away. As I remember (have no stack at the moment), the illegal instruction appears somewhere in RocksDb initialization. See: ECR-1558 * Upgrade to Exonum 0.8 [ECR-1620] * Improve the documentation of a test. (#284) * Change Java serialization to protobuf and use owner’s public key as wallet id [ECR-1493, ECR-1494] - Change Java Serialization of transactions and wallets to protobuf. - Wallet String name is changed to the wallet owner’s public key. - Fix a bug in send money transaction when a transaction with the sender equal to the receiver resulted in doubling the balance of that wallet. PR: #272 * Messages payload length fix [ECR-1624]: 'payload length' field in the message header used to be equal to 'body' length in bytes, while it should be equal to the size of the whole message (header + body + signature). Also, improve the tests and exception messages. PR: #285 * Fix the service API path: (#288) The service API path must start with a slash to be correctly mounted. * Fix the transaction location URL [ECR-1648] (#293). The base path of explorer was changed in Exonum 0.7 from ""/api/system/v1" to "/api/explorer/v1/". * Add Javadoc plugin [ECR-1510] (#290): Javadoc generation does not happen during normal builds and must be triggered manually. JARs with the Javadoc for all modules can be generated with $ mvn javadoc:jar * Fix build on Travis CI [ECR-1658] (#292): * Build the dependencies instead of using installed in the system to prevent illegal instructions appearing in our artefacts (the actual cause is still unknown). * Disable caching of rust/target for it takes at least 2.5G, which is too much for Travis to archive and upload. * Add an Exonum App [ECR-869, ECR-867] (#238): Implemented JavaServiceRuntime abstraction around JVM to configure and run Java services. JavaServiceRuntime is a ServiceFactory, allowing to configure Java services in the same way as any other Exonum service. Also includes EJB App - a Rust application to configure and run an Exonum node with a specific Java service. * Add LICENSE and license headers in every source file [ECR-1499] (#295): Also added a script generating a text file with third-party licenses of Rust dependencies: ./generate_licenses.sh * Prepare the build configuration for a release [ECR-1526] (#298): Add required plugins to prepare artefacts for a publication to the repository. * Update native-proxies branch to Exonum 0.8.1 [ECR-1157] (#299). * Fix NodeProxy so that it can create a View * Remove 'provided' scope of the system dependencies of a service: (#301) Until we ship an app that includes these dependencies, they must be available on the classpath of the service (the only configuration option that we will have at 0.1 release). * Remove unused imports. * Use the Java home that Maven reports in the scripts setting LD_LIBRARY_PATH * Fix the Exception class that we use in tests: The mocked methods can not throw checked exceptions. * Skip all tests until the bug with loading shared libraries is fixed. * Add a tutorial describing how to launch a node with a Java service (#300) * Fix the application name and the script to start the crypto demo. * Update the project documents [ECR-486, ECR-1515, ECR-1516] (#296): Add a contribution guide, clean up the readme, add a roadmap. * Fix the formatting. * Fix anchors in the contribution guide. * Change travis-ci.com to travis-ci.org (#305)
Overview
Payload length in message header was corresponding to body length, while it should be equal to the size of a whole message (header + body + signature).
See: https://jira.bf.local/browse/ECR-1624
Definition of Done