Memo padding edge cases#162
Merged
xeroc merged 4 commits intobitshares:developfrom Dec 5, 2018
Merged
Conversation
This simplifies the code and removes the ambiguity with "last byte" fetching. If the message contains unicode characters in the end, they will be dealt with correctly now.
This avoids weird edge cases, when the message is exactly the length
of 16 byte span, and the last byte then gets erroneously treated
as padding marker.
Note: messages with length of 15 (or 16+15, etc) bytes still
get encoded as before, it's only when the edge is pushed
and there's no place for a "padding marker", we add 16 more
bytes (yes, they all consist of the same 0x0F byte).
Example:
"1234567890A" = "1234567890A" + ("\x0F" * 1)
"1234567890AA" = "1234567890AA" + ("\x0F" * 16)
Note: bitcoin-core does it the same way, which makes sense.
(As it's the only sane way to distinguish between actual last byte
of the message and padding markers.)
Codecov Report
@@ Coverage Diff @@
## develop #162 +/- ##
========================================
Coverage 44.61% 44.61%
========================================
Files 34 34
Lines 3340 3340
Branches 723 723
========================================
Hits 1490 1490
Misses 1636 1636
Partials 214 214Continue to review full report at Codecov.
|
Member
|
Wow. Excellent catch! Thank you! |
xeroc
added a commit
that referenced
this pull request
Dec 19, 2018
Release 0.3.0rc1 This release moves implementation details to pygraphene to improve maintainability f9fb23b (HEAD -> master) Merge branch 'release/0.3.0rc1' f779c47 (release/0.3.0rc1) Version bump 1e7d98c (develop) Merge branch 'feature/cleanup-txbuilder' into develop 2695525 Fix messages and account b67724c Bump requirement for pygraphene 431582f Updates to documentation 3a6c491 remove obsolete modules 1a06cfc Update documentation 3918218 cleanup 43a2170 Simplify Key classes 960f3aa Remove logging module acc54d5 move general stuff to graphene 7e5d506 (origin/develop) Cleanup base.memo 2a0b813 Simplyify dealing with Prefixes 3e15f87 Improve class dealing and unittesting 055635f Fix unittesting e13f77f define required type_id 3a9f16d Migrate common stuff to python-graphene 9c9b15a Separation of wallet.py 4b72616 Fix fox.ini with blake 3bb9b8b Fix __init__ 2e48137 pre-commit installation 8b5294d testing pre-commit 80013b1 linting c96dac6 Isort 812e6e3 Black reformatting 20258e2 Re-enable computation of mcr 5efb1d4 Merge pull request #162 from jhtitor/memo_pad 4a633ca Merge pull request #160 from Algruun/master e2ef0a4 Verify memo checksum during decoding. 1c50b37 Always add final padding during memo encryption. b3f1bda Make memo _pad work with bytes, instead of strings. 588b8e7 Add memo tests for padding edge cases. f41bd77 Bid Collateral Operation 54dcfbf Bid Collateral Operation 0b2b137 Message, ensure we can sign on different blockchain too 9644bf5 Ensure we can deal with \r as well 70b8260 [message] Better exception message cd91155 Also store plain message and meta separately dae6918 After verification fill in class members bc73a8a #128 Proposal fix, now, at least method accountopenorders won't raise KeyError "limit_orders" 35a3aaf Fix #151 ebb1f2d Add Hackthedex link in readme.md #153 cf8fafb Ensure we can verify messages on different networks too 8d66b43 Fix getActiveKeyForAccount failing when multiple keys installed 3aa81cc Makefile rename back temporary test 54600bb use twine for pypi upload e053940 Merge tag '0.2.1' into develop
xeroc
added a commit
that referenced
this pull request
Mar 4, 2019
Release 0.3.0 The major change of this release was to merge common code with other graphene-based blockchains into python-graphene to reduce redunance and simply code maintainability. Features -------- d1484a0 Fix issue with backend showing wrong percentage change after a while ba23069 Allow to create an account that only references to another account 23b2c7a Implement issuing of shares as part of Asset() ab40514 Simplify use of custom authorties c349316 Merge pull request #179 from jhtitor/few_asset_ops 9c2c3bf Add test for Asset_claim_pool operation. 5ad23c4 Add asset_claim_pool operation. 42deeda Add tests for global_settle and claim_fees operations. dd4cb96 Add asset_global_settle and asset_claim_fees operations. 0a25a4d New operation 1f1e355 Merge 'upstream/develop' into few_asset_ops 681b0fa Implement asset-settle 74fa037 Finish up unittests using new caching system 63a4030 Import Object from graphenecommon 43a2170 Simplify Key classes acc54d5 move general stuff to graphene 2a0b813 Simplyify dealing with Prefixes 3a9f16d Migrate common stuff to python-graphene 9c9b15a Separation of wallet.py f41bd77 Bid Collateral Operation 54dcfbf Bid Collateral Operation Fixes ----- 436fb40 Fix Amount in Open Order and improve unittest (Fix #76) acf9a29 Fix #180 b061399 Fix #185 b64f222 fix precommit to use python3 bf6a3dc Merge pull request #171 from bitfag/fix-order-rounding 6cc984a Fix Object() class and add unit test abf4046 Fix incorrect initialization of FilledOrder and Order d52f7da Fix price feeds ac91f9e Fix buy/sell of near-precision amounts 2695525 Fix messages and account 055635f Fix unittesting 4b72616 Fix fox.ini with blake 3bb9b8b Fix __init__ bc73a8a #128 Proposal fix, now, at least method accountopenorders won't raise KeyError "limit_orders" 35a3aaf Fix #151 8d66b43 Fix getActiveKeyForAccount failing when multiple keys installed 61712db Ensure we do have a default expiration time for new orders 949850b Properly initialize blockchain instance 8f9bc23 Also import the exceptions 4d27455 Also import the exceptions 1e7d98c Merge branch 'feature/cleanup-txbuilder' into develop cf8fafb Ensure we can verify messages on different networks too e13f77f define required type_id 20258e2 Re-enable computation of mcr 5efb1d4 Merge pull request #162 from jhtitor/memo_pad 4a633ca Merge pull request #160 from Algruun/master e2ef0a4 Verify memo checksum during decoding. 1c50b37 Always add final padding during memo encryption. b3f1bda Make memo _pad work with bytes, instead of strings. 588b8e7 Add memo tests for padding edge cases. 0b2b137 Message, ensure we can sign on different blockchain too 9644bf5 Ensure we can deal with \r as well cd91155 Also store plain message and meta separately dae6918 After verification fill in class members 3aa81cc Makefile rename back temporary test 54600bb use twine for pypi upload Testnet ------- 1ec6faa Add HTLC class e4d743d HTLC implementation 13e0168 Switch tests back to main net (breaks HLTC unit tests since ops are not available on mainnet) 65c451b Implement HTLC base operations Dependencies ------------ 802d576 bump dependency for pygraphene b67724c Bump requirement for pygraphene Code Style and Cleanups ----------------------- c95d093 linting 0e155e9 tox and requirements for black and flake8 2e48137 pre-commit installation 8b5294d testing pre-commit 80013b1 linting c96dac6 Isort 812e6e3 Black reformatting 3a6c491 remove obsolete modules 3918218 cleanup 960f3aa Remove logging module 7e5d506 Cleanup base.memo 3e15f87 Improve class dealing and unittesting Documentation ------------- 431582f Updates to documentation 1a06cfc Update documentation ebb1f2d Add Hackthedex link in readme.md #153 70b8260 [message] Better exception message
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There are currently several inter-related bugs in the memo padding/unpadding mechanism.
_unpadmethod tries to guess if the final byte of the message is a padding marker or an actual data byte.this guesswork can fail, if, for example, the message ends with
\x02\x02(yes, that should be possible, as it's a valid unicode string).This can fail if
swas unicode- (and not ascii-) encoded. If we adjust it tothen,
bytes(s[-1], 'utf8')might result in a multi-byte sequence and will break the call tounpack('B'), which expects a single byte.To deal with both those cases, I've converted
_unpadto use raw bytes, which removes any ambiguity with the final byte and re-encoding.However, this still leaves another important bug: the guesswork should never happen at all! The messages prepared by
encode_memoare not properly padded.Here, the comment claims "this adds 16 bytes even if not required ", however the exact opposite is happening! The code above "would not add padding to 16 byte-sized message, even as it IS required".
Consider the string
abcdefghijÛ. There are 10 ascii characters, 1 unicode 2-byte character and 4 byte checksum (not shown here). Thus, 10+2+4 = 16. So, we're hitting an edge case and the above code WILL NOT add any padding. Now, the message ends withÛand the decoder has to guess if it's a final marker or not, as described above.I've taken a look at
bitshares-core, in it, there's no ambiguity or guesswork involved: the padding algorithm would just add another 16 bytes (full of padding markers) to such string.To reiterate: "abcdefghijÛ" encoded with python-bitshares = 16 bytes, "abcdefghijÛ" encoded with bitshares-core = 32 bytes.
After applying the patches in this PR: