LBRYCommunity/lbrycrd
1
0
Fork 0
Code Issues 29 Pull requests 1 Packages Projects Releases 32 Wiki Activity

Unify claimtrie tests, add some additional root hash checks #181

Closed
BrannonKing wants to merge 291 commits from unify_claimtrie_tests into master
pull from: unify_claimtrie_tests
merge into: LBRYCommunity:master
Conversation 6 Commits 291 Files changed 118 +2159 -2883
BrannonKing commented 2018-07-27 00:24:11 +02:00 (Migrated from github.com)
Copy link

As part of the review, look for the comment "tx1MerkleHash doesn't match right here" and tell me if that's expected.

As part of the review, look for the comment "tx1MerkleHash doesn't match right here" and tell me if that's expected.
kaykurokawa (Migrated from github.com) reviewed 2018-07-27 00:24:11 +02:00
lbrynaut commented 2018-07-27 00:33:17 +02:00 (Migrated from github.com)
Copy link

If this isn't ready yet, consider adding WIP (for work in progress to the PR name).

If this isn't ready yet, consider adding WIP (for work in progress to the PR name).
lbrynaut (Migrated from github.com) reviewed 2018-07-27 00:34:06 +02:00
lbrynaut (Migrated from github.com) left a comment
Copy link

Will review more later

Will review more later
src/test/claimtriebranching_tests.cpp
@ -656,2 +664,3 @@
BOOST_CHECK(is_best_claim("test",u3));
fixture.DecrementBlocks(11);
BOOST_CHECK_EQUAL(2U, pclaimTrie->getClaimsForName("test").claims.size());
fixture.DecrementBlocks(11);
lbrynaut (Migrated from github.com) commented 2018-07-27 00:32:29 +02:00
Copy link

The previous test names were far more descriptive when tracking down logged errors. It described module and the test name, not a name with "_test" appended (which is non-informative, given that all unit tests are tests).

The previous test names were far more descriptive when tracking down logged errors. It described module and the test name, not a name with "_test" appended (which is non-informative, given that all unit tests are tests).
lbrynaut commented 2018-07-27 00:35:23 +02:00 (Migrated from github.com)
Copy link

Also, just a general question -- why are these being merged? I liked having the older tests separate from the more recent ones, which can help find regressions (have actually found several while I was working on the normalization).

Also, just a general question -- why are these being merged? I liked having the older tests separate from the more recent ones, which can help find regressions (have actually found several while I was working on the normalization).
BrannonKing (Migrated from github.com) reviewed 2018-07-27 03:26:29 +02:00
src/test/claimtriebranching_tests.cpp
@ -656,2 +664,3 @@
BOOST_CHECK(is_best_claim("test",u3));
fixture.DecrementBlocks(11);
BOOST_CHECK_EQUAL(2U, pclaimTrie->getClaimsForName("test").claims.size());
fixture.DecrementBlocks(11);
BrannonKing (Migrated from github.com) commented 2018-07-27 03:26:29 +02:00
Copy link

The filename is actually part of the displayed name for both error and when running it manually; it ends up being "claimtriebranching_tests/claimtriebranching_support", etc. Hence, I didn't see the purpose in keeping the namespace duplicated in the test name. I'm not crazy about ending them with "_test", either. It just looked like that's what some other files did. I still think most of the names are not helpful. All of the tests cover more ground than their name implies, and many of the test names are too generic.

The filename is actually part of the displayed name for both error and when running it manually; it ends up being "claimtriebranching_tests/claimtriebranching_support", etc. Hence, I didn't see the purpose in keeping the namespace duplicated in the test name. I'm not crazy about ending them with "_test", either. It just looked like that's what some other files did. I still think most of the names are not helpful. All of the tests cover more ground than their name implies, and many of the test names are too generic.
BrannonKing commented 2018-07-27 03:30:47 +02:00 (Migrated from github.com)
Copy link

As for the "why" question, @kaykurokawa can say why he filed #83 . There is 200 lines less code than there was before the test merge, and the old tests are still covering everything that they did before. It's just not so obvious now that they're older.

As for the "why" question, @kaykurokawa can say why he filed #83 . There is 200 lines less code than there was before the test merge, and the old tests are still covering everything that they did before. It's just not so obvious now that they're older.
BrannonKing commented 2018-07-27 18:52:58 +02:00 (Migrated from github.com)
Copy link

I think I finally got the right format command figured out. I'll push up that commit to make sure it builds on Travis. I'm happy to squash this before we merge, but I think the different commits allow you to more easily see what was added to test hashes and tree counts.

I think I finally got the right format command figured out. I'll push up that commit to make sure it builds on Travis. I'm happy to squash this before we merge, but I think the different commits allow you to more easily see what was added to test hashes and tree counts.
lbrynaut commented 2018-07-28 00:13:15 +02:00 (Migrated from github.com)
Copy link

I think I finally got the right format command figured out. I'll push up that commit to make sure it builds on Travis. I'm happy to squash this before we merge, but I think the different commits allow you to more easily see what was added to test hashes and tree counts.

Everyone has different opinions here, but my take is that pull requests with many commits are just distracting since either the pull request addresses the issue or not. Most times when you see a stack of commits, they are exactly things like "ran formatting tool", "updated test", "something else useless", etc. In reality, reviewers look at the change-set as a whole because the PR is more or less all or nothing. Cherry-picking individual commits from a single PR is very rare -- I'd dare say it never happens. Instead you see even more garbage commits like "updated test", " fixed the issue added by test", "reverted last change", "really updated test".

Developers tend to overvalue the sequence of how code arrived at a particular place -- for example, a common argument I hear for not squashing is that so that a particular developer can remember the process of making the changes, or some other sentimental/historical reason. The problem is that anyone can do that in a local tree, but it's meaningless (and distracting) to other developers, the ones reviewing and/or maintaining it -- who are not interested in that sequence, and it actually makes history tracking somewhat more difficult because instead of a concrete place where PR X was merged (which can be reviewed/edited/undone), you have to potentially suffer through reviewing 40+ commits to find the actual spot where something happened/broke/changed (including all the 10+ other commits in there where it was twiddled back and forth for whatever reason during the 'process').

This is a bit of rant, not particularly just about your comment @BrannonKing. I find clean source histories very important, particularly when we are looking to be tracking an upstream. It's the entire reason that git is more powerful than other scms (the ability to hack/clean the timeline). I'm just advocating for professional standards on the main public repo. Of course, anyone can do whatever they'd like on their personal repos.

@kaykurokawa and I largely agreed that squashing the commits was the least noisy (last we talked about it, although it's been a while). If you find that this is not the right thing to do in some cases, just make the case. But keep the above in mind. There's nothing wrong with formatting PRs, but they would ideally just be formatting -- not formatting and overhaul subsystem X at the same time.

> I think I finally got the right format command figured out. I'll push up that commit to make sure it builds on Travis. I'm happy to squash this before we merge, but I think the different commits allow you to more easily see what was added to test hashes and tree counts. Everyone has different opinions here, but my take is that pull requests with many commits are just distracting since either the pull request addresses the issue or not. Most times when you see a stack of commits, they are exactly things like "ran formatting tool", "updated test", "something else useless", etc. In reality, reviewers look at the change-set as a whole because the PR is more or less all or nothing. Cherry-picking individual commits from a single PR is very rare -- I'd dare say it never happens. Instead you see even more garbage commits like "updated test", " fixed the issue added by test", "reverted last change", "really updated test". Developers tend to overvalue the sequence of how code arrived at a particular place -- for example, a common argument I hear for not squashing is that so that a particular developer can remember the process of making the changes, or some other sentimental/historical reason. The problem is that anyone can do that in a local tree, but it's meaningless (and distracting) to other developers, the ones reviewing and/or maintaining it -- who are not interested in that sequence, and it actually makes history tracking somewhat more difficult because instead of a concrete place where PR X was merged (which can be reviewed/edited/undone), you have to potentially suffer through reviewing 40+ commits to find the actual spot where something happened/broke/changed (including all the 10+ other commits in there where it was twiddled back and forth for whatever reason during the 'process'). This is a bit of rant, not particularly just about your comment @BrannonKing. I find clean source histories very important, particularly when we are looking to be tracking an upstream. It's the entire reason that git is more powerful than other scms (the ability to hack/clean the timeline). I'm just advocating for professional standards on the main public repo. Of course, anyone can do whatever they'd like on their personal repos. @kaykurokawa and I largely agreed that squashing the commits was the least noisy (last we talked about it, although it's been a while). If you find that this is not the right thing to do in some cases, just make the case. But keep the above in mind. There's nothing wrong with formatting PRs, but they would ideally just be formatting -- not formatting and overhaul subsystem X at the same time.
BrannonKing (Migrated from github.com) reviewed 2018-07-28 00:58:02 +02:00
src/test/claimtriebranching_tests.cpp
@ -1067,0 +1463,4 @@
fixture.CommitTx(tx1);
fixture.IncrementBlocks(1, true);
BOOST_CHECK(pclaimTrie->getInfoForName(sName1, val));
BrannonKing (Migrated from github.com) commented 2018-07-28 00:58:02 +02:00
Copy link

This mismatch on the end is what was causing the Travis-CI test failure. Strange thing: it doesn't fail with the GCC 7 builds, with or without optimizations. Only the GCC 4 builds fail.

This mismatch on the end is what was causing the Travis-CI test failure. Strange thing: it doesn't fail with the GCC 7 builds, with or without optimizations. Only the GCC 4 builds fail.
lbrynaut (Migrated from github.com) reviewed 2018-07-28 01:58:25 +02:00
src/test/claimtriebranching_tests.cpp
@ -1067,0 +1463,4 @@
fixture.CommitTx(tx1);
fixture.IncrementBlocks(1, true);
BOOST_CHECK(pclaimTrie->getInfoForName(sName1, val));
lbrynaut (Migrated from github.com) commented 2018-07-28 01:58:25 +02:00
Copy link

Interesting. If you're able to track down why that is, it may save us some headaches in the future. Once we are inline with upstream, gcc 5/6/7/8 becomes a viable option, perhaps even a minimum supported version.

Interesting. If you're able to track down why that is, it may save us some headaches in the future. Once we are inline with upstream, gcc 5/6/7/8 becomes a viable option, perhaps even a minimum supported version.
lbrynaut (Migrated from github.com) reviewed 2018-07-28 02:03:15 +02:00
src/test/claimtriebranching_tests.cpp
@ -1067,0 +1463,4 @@
fixture.CommitTx(tx1);
fixture.IncrementBlocks(1, true);
BOOST_CHECK(pclaimTrie->getInfoForName(sName1, val));
lbrynaut (Migrated from github.com) commented 2018-07-28 02:03:15 +02:00
Copy link

This looks like a bug? std::vector<unsigned char>(sValue1.begin(), sValue2.end())

This looks like a bug? ```std::vector<unsigned char>(sValue1.begin(), sValue2.end())```
lbrynaut (Migrated from github.com) reviewed 2018-07-28 02:04:45 +02:00
src/test/claimtriebranching_tests.cpp
@ -1067,0 +1463,4 @@
fixture.CommitTx(tx1);
fixture.IncrementBlocks(1, true);
BOOST_CHECK(pclaimTrie->getInfoForName(sName1, val));
lbrynaut (Migrated from github.com) commented 2018-07-28 02:04:45 +02:00
Copy link

Also, does the construction form make any difference?

Changing something like:

std::vector<unsigned char>(sName1.begin(), sName1.end())

to

std::vector<unsigned char>(sName1.data(), sName1.data() + sName1.size())
Also, does the construction form make any difference? Changing something like: ``` std::vector<unsigned char>(sName1.begin(), sName1.end()) ``` to ``` std::vector<unsigned char>(sName1.data(), sName1.data() + sName1.size()) ```
lbrynaut (Migrated from github.com) reviewed 2018-07-28 02:06:36 +02:00
src/test/claimtriebranching_tests.cpp
@ -1067,0 +1463,4 @@
fixture.CommitTx(tx1);
fixture.IncrementBlocks(1, true);
BOOST_CHECK(pclaimTrie->getInfoForName(sName1, val));
lbrynaut (Migrated from github.com) commented 2018-07-28 02:06:36 +02:00
Copy link

On second read ... are you just saying that the mismatch is what the actual problem is and later compilers don't notice it? We can play with the warning levels if so later.

On second read ... are you just saying that the mismatch is what the actual problem is and later compilers don't notice it? We can play with the warning levels if so later.
BrannonKing (Migrated from github.com) reviewed 2018-07-28 02:25:03 +02:00
src/test/claimtriebranching_tests.cpp
@ -1067,0 +1463,4 @@
fixture.CommitTx(tx1);
fixture.IncrementBlocks(1, true);
BOOST_CHECK(pclaimTrie->getInfoForName(sName1, val));
BrannonKing (Migrated from github.com) commented 2018-07-28 02:25:03 +02:00
Copy link

Neither compiler caught the error. It was a runtime failure. When, compiled with GCC7, though, the unit test passes with no runtime failure notwithstanding the wrong end sentinel. I tried to do your data() + size() suggestion but that won't compile; it definitely wants an end sentinel, not a real pointer.

Neither compiler caught the error. It was a runtime failure. When, compiled with GCC7, though, the unit test passes with no runtime failure notwithstanding the wrong end sentinel. I tried to do your `data() + size()` suggestion but that won't compile; it definitely wants an end sentinel, not a real pointer.
lbrynaut (Migrated from github.com) reviewed 2018-07-28 02:40:05 +02:00
src/test/claimtriebranching_tests.cpp
@ -1067,0 +1463,4 @@
fixture.CommitTx(tx1);
fixture.IncrementBlocks(1, true);
BOOST_CHECK(pclaimTrie->getInfoForName(sName1, val));
lbrynaut (Migrated from github.com) commented 2018-07-28 02:40:05 +02:00
Copy link

Ah right, it would require a temporary, (I hacked it since we're not using c++11). The preferred syntax (c++11) would be:

{ sName1.data(), sName1.data() + sName1.size() }

The test without is something like:

std::vector<unsigned char> foo(sName1.data(), sName1.data() + sName1.size())

...
<< foo << other_stuff;
Ah right, it would require a temporary, (I hacked it since we're not using c++11). The preferred syntax (c++11) would be: ``` { sName1.data(), sName1.data() + sName1.size() } ``` The test without is something like: ``` std::vector<unsigned char> foo(sName1.data(), sName1.data() + sName1.size()) ... << foo << other_stuff; ```
lbrynaut (Migrated from github.com) reviewed 2018-07-28 02:41:03 +02:00
src/test/claimtriebranching_tests.cpp
@ -1067,0 +1463,4 @@
fixture.CommitTx(tx1);
fixture.IncrementBlocks(1, true);
BOOST_CHECK(pclaimTrie->getInfoForName(sName1, val));
lbrynaut (Migrated from github.com) commented 2018-07-28 02:41:02 +02:00
Copy link

BTW, if the test passes with that bug, there's another bug at the surface.

BTW, if the test passes with that bug, there's another bug at the surface.
BrannonKing (Migrated from github.com) reviewed 2018-07-28 18:53:31 +02:00
src/test/claimtriebranching_tests.cpp
@ -1067,0 +1463,4 @@
fixture.CommitTx(tx1);
fixture.IncrementBlocks(1, true);
BOOST_CHECK(pclaimTrie->getInfoForName(sName1, val));
BrannonKing (Migrated from github.com) commented 2018-07-28 18:53:31 +02:00
Copy link

I didn't notice the data() on the first parameter previously. This does compile and also does not throw a runtime error in GCC7: ... std::vector<unsigned char>(sValue1.data(), sValue2.data() + sValue2.size()) .... It probably works because sValue1 and sValue2 are in order on the stack; you end up with sValue1 prepended to sValue2 with some junk in the middle, but we never verify the value.

I didn't notice the `data()` on the first parameter previously. This does compile and also does not throw a runtime error in GCC7: `... std::vector<unsigned char>(sValue1.data(), sValue2.data() + sValue2.size()) ...`. It probably works because sValue1 and sValue2 are in order on the stack; you end up with sValue1 prepended to sValue2 with some junk in the middle, but we never verify the value.
BrannonKing commented 2018-07-31 02:36:55 +02:00 (Migrated from github.com)
Copy link

I believe that I now understand my question where the claimtrie merkle root hash didn't match: the height (er, block number) where the transaction is activated (known as node->nHeightOfLastTakeover) is also included in the hash input. The upper one is activated at height 141; the lower at height 153.

I believe that I now understand my question where the claimtrie merkle root hash didn't match: the height (er, block number) where the transaction is activated (known as node->nHeightOfLastTakeover) is also included in the hash input. The upper one is activated at height 141; the lower at height 153.
👍 1

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
kaykurokawa
Labels
Clear labels   
area: devops

   
area: discovery

   
area: docs

   
area: livestream

   
area: proposal

Work is part of a proposal

  
consider soon

Discuss this issue at the next planning meeting, then remove this label

  
Epic

   
good first issue

   
hacktoberfest

Welcome to Hacktoberfest

  
hard fork

   
help wanted

   
icebox

Long-term storage

  
Invalid

   
level: 0

   
level: 1

No knowledge of the existing code required

  
level: 2

Some knowledge of the existing code is recommended

  
level: 3

Significant knowledge of the existing code is recommended

  
level: 4

Intimate knowledge of the existing code is recommended

  
needs: exploration

Solution unclear, needs research

  
needs: grooming

Issue needs to be groomed before work can start

  
needs: priority

Priority level needs to be defined

  
needs: repro

Issue needs step-by-step instructions on how to reproduce in latest code

  
needs: tech design

Needs technical design signoff before implementation can begin

  
on hold

Temporarily paused

  
priority: blocker

Issue is blocking release, do ASAP

  
priority: high

Work needs to be moved into sprint ASAP

  
priority: low

Work should be done but can stay in the backlog for now

  
priority: medium

Work needs to be done within 2-3 sprints

  
resilience

   
soft fork

   
Tom's Wishlist

   
type: bug

Existing functionality is wrong or broken

  
type: discussion

A conversation. No specific changes requested

  
type: improvement

Existing (or partially existing) functionality needs to be changed

  
type: new feature

New functionality that does not exist yet

  
type: refactor

Minimal user-visible changes, but significant internal work

  
type: task

Either work that's not related to the code, or a small chore that does not fit into other categories

  
type: testing

Solution needs additional user testing

  
unplanned

Work that was not planned into the spirnt, took priority over planned work

  
work in progress

The proposed solution is still incomplete.

No labels
area: devops
area: discovery
area: docs
area: livestream
area: proposal
consider soon
Epic
good first issue
hacktoberfest
hard fork
help wanted
icebox
Invalid
level: 0
level: 1
level: 2
level: 3
level: 4
needs: exploration
needs: grooming
needs: priority
needs: repro
needs: tech design
on hold
priority: blocker
priority: high
priority: low
priority: medium
resilience
soft fork
Tom's Wishlist
type: bug
type: discussion
type: improvement
type: new feature
type: refactor
type: task
type: testing
unplanned
work in progress
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbrycrd#181
No description provided.
Delete branch "unify_claimtrie_tests"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?