node: event tree commit #201

tynes · 2019-06-19T06:48:36Z

Emit an event over websockets for each time that a new tree root is set.
The event is tree commit on the topic chain.

The event is added to chain.setBestChain which is called on every valid block that comes in. This means that this event could be triggered when a block that is not part of the best chain is received by the node.

Should we add additional information regarding if it is part of the best chain?
Or only make it so that the event is triggered when its part of the main chain?

codecov-io · 2019-06-19T06:51:42Z

Codecov Report

Merging #201 into master will increase coverage by 0.58%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #201      +/-   ##
==========================================
+ Coverage   52.96%   53.55%   +0.58%     
==========================================
  Files         129      129              
  Lines       35746    35755       +9     
  Branches     6026     6029       +3     
==========================================
+ Hits        18932    19147     +215     
+ Misses      16814    16608     -206

Impacted Files	Coverage Δ
lib/blockchain/chain.js	`61.03% <100.00%> (+0.58%)`	⬆️
lib/node/http.js	`50.96% <100.00%> (+3.75%)`	⬆️
lib/script/script.js	`57.92% <0.00%> (-0.08%)`	⬇️
lib/node/node.js	`64.00% <0.00%> (+1.00%)`	⬆️
lib/blockchain/chainentry.js	`59.74% <0.00%> (+1.29%)`	⬆️
lib/primitives/tx.js	`74.90% <0.00%> (+1.93%)`	⬆️
lib/primitives/block.js	`71.72% <0.00%> (+2.61%)`	⬆️
lib/blockchain/chaindb.js	`67.04% <0.00%> (+3.31%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1c0281...473dbe6. Read the comment docs.

boymanjor · 2019-06-19T18:44:56Z

Should we add additional information regarding if it is part of the best chain?
Or only make it so that the event is triggered when its part of the main chain?

To be clear, "best" and "main" are synonymous in this context, correct? You used them separately; I want to be sure my understanding is accurate.

Are we sure that setBestChain handles blocks from alternate chains? The jsdoc comments lead me to believe this is true, but upon a first walkthrough of the code path, I cannot confirm the validity of this assumption.

setBestChain only gets called here. The enclosing if statement either calls setBestChain or saveAlternate. The former emits the tip, block, and now information about the tree's root. The latter only emits competitor which signifies an alternate chain. Even the name of the method implies that this is only used to connect a block to the best chain. If you agree with this analysis we should update the comment and leave this change as-is. If I am wrong, I think we should indicate whether or not the tree commit event is tied to the best chain.

tynes · 2019-06-21T00:32:34Z

Safety Report

The call stack for a block being added to the chain is as follows:

Chain.add - source
Chain._add - source
Chain.connect - source
Chain.setBestChain - source

The logic inside of Chain.connect determines if Chain.setBestChain will be called or if Chain.saveAlternate is called.

The relevant codepath:

hsd/lib/blockchain/chain.js

Lines 2008 to 2021 in e1c0281

    
           // Create a new chain entry. 
        
           const entry = ChainEntry.fromBlock(block, prev); 
        
           // The block is on a alternate chain if the 
        
           // chainwork is less than or equal to 
        
           // our tip's. Add the block but do _not_ 
        
           // connect the inputs. 
        
           if (entry.chainwork.lte(this.tip.chainwork)) { 
        
             // Save block to an alternate chain. 
        
             await this.saveAlternate(entry, block, prev, flags); 
        
           } else { 
        
             // Attempt to add block to the chain index. 
        
             await this.setBestChain(entry, block, prev, flags); 
        
           }

If the incoming block's chainwork is less than or equal to the current tips chainwork, then Chain.saveAlternate is called. The only way that Chain.setBestChain is called is if the incoming block has greater chain work. This means that the jsdoc is incorrect for setBestChain regarding the line "This is called on every valid block that comes in.". It is only called for blocks that extend
the current chain and not for alternative blocks.

hsd/lib/blockchain/chain.js

Lines 1611 to 1625 in e1c0281

    
             /** 
        
              * Set the best chain. This is called on every valid block 
        
              * that comes in. It may add and connect the block (main chain), 
        
              * save the block without connection (alternate chain), or 
        
              * reorganize the chain (a higher fork). 
        
              * @private 
        
              * @param {ChainEntry} entry 
        
              * @param {Block} block 
        
              * @param {ChainEntry} prev 
        
              * @param {Number} flags 
        
              * @returns {Promise} 
        
              */ 
        
             async setBestChain(entry, block, prev, flags) { 
        
               // A higher fork has arrived.

You can see here that the jsdoc is correct regarding setBestChain as a private function. The
only place that it is invoked is withing lib/blockchain/chain.js inside of Chain.connect.

$ basename $PWD
hsd
$ grep -rn setBestChain lib
lib/blockchain/chain.js:1464:    // That will be done outside in setBestChain.
lib/blockchain/chain.js:1624:  async setBestChain(entry, block, prev, flags) {
lib/blockchain/chain.js:2020:      await this.setBestChain(entry, block, prev, flags);

After doing this review, I believe that it is safe to include the event emitting exactly where it is
currently in this pull request. Chain.setBestChain is not called for every block, just blocks
extending the best chain. I will edit the commit messages to remove the safety warning
and include this report in the commit message.

Additional Work

I think that reorgs should trigger the event tree commit as well. Chain.connect is only
called once for every block, if there is a reorg the codepath is as follows:

Chain.add - source
Chain._add - source
Chain.connect - source
Chain.setBestChain - source
Chain.reorganize - source
Chain.reconnect - source

A tree commit event could be added inside of Chain.reconnect as well, it also emits events:

hsd/lib/blockchain/chain.js

Lines 1605 to 1606 in e1c0281

    
           this.emit('tip', entry); 
        
           this.emit('reconnect', entry, block);

This will ensure that the event is emitted when reorgs happen and different tree roots are committed.

tynes · 2019-06-21T00:34:23Z

Should we add additional information regarding if it is part of the best chain?
Or only make it so that the event is triggered when its part of the main chain?

To be clear, "best" and "main" are synonymous in this context, correct? You used them separately; I want to be sure my understanding is accurate.

Yes that is what I meant. Sorry for using two words to mean the same thing.

If you agree with this analysis we should update the comment and leave this change as-is

I agree with the analysis and laid out evidence above. Will update the commit appropriately.

chjj · 2019-06-21T12:49:03Z

To make things clear: any time verifyContext is called, the block is being added to the main chain. The current code looks correct to me. There's probably a number of incorrect jsdocs in the codebase due to refactors and whatnot (I think I'm getting sick of maintaining jsdoc =/).

tynes · 2019-06-21T17:02:46Z

Just pushed up a fix to the jsdoc.

I think I'm getting sick of maintaining jsdoc =/

What do you think of having high level READMEs in each package similar to btcd/lnd?
The one thing that I do like about jsdoc is the type hints. Without those, things could become pretty messy. Can't wait until Typescript is Javascript.

lib/blockchain/chain.js

Emit an event named 'tree commit' from the chain on the 'chain' topic. It emits three items, the newly comitted tree root (which is not included in a block until the next block) along with the current block and the chain entry. This happens in the `setBestChain` method on the chain. The computation to determine the event is: ``` if ((entry.height % this.network.names.treeInterval) === 0) ``` The call stack for a block being added to the chain is as follows: - `Chain.add` - [source](https://github.com/handshake-org/hsd/blob/e1c0281dd9e9bbbd23f5cd832c82ebe3808e9b4f/lib/blockchain/chain.js#L1885) - `Chain._add` - [source](https://github.com/handshake-org/hsd/blob/e1c0281dd9e9bbbd23f5cd832c82ebe3808e9b4f/lib/blockchain/chain.js#L1904) - `Chain.connect` - [source](https://github.com/handshake-org/hsd/blob/e1c0281dd9e9bbbd23f5cd832c82ebe3808e9b4f/lib/blockchain/chain.js#L1979) - `Chain.setBestChain` - [source](https://github.com/handshake-org/hsd/blob/e1c0281dd9e9bbbd23f5cd832c82ebe3808e9b4f/lib/blockchain/chain.js#L1624) The logic inside of `Chain.connect` determines if `Chain. setBestChain` will be called or if `Chain.saveAlternate` is called. The relevant codepath: https://github.com/handshake-org/hsd/blob/e1c0281dd9e9bbbd23f5cd832c82ebe3808e9b4f/lib/blockchain/chain.js#L2008-L2021 If the incoming block's chainwork is less than or equal to the current tips chainwork, then `Chain.saveAlternate` is called. The only way that `Chain.setBestChain` is called is if the incoming block has greater chain work. This means that the jsdoc is __incorrect__ for `setBestChain` regarding the line `"This is called on every valid block that comes in."`. It is only called for blocks that extend the current chain and not for alternative blocks. https://github.com/handshake-org/hsd/blob/e1c0281dd9e9bbbd23f5cd832c82ebe3808e9b4f/lib/blockchain/chain.js#L1611-L1625 You can see here that the jsdoc is correct regarding `setBestChain` as a private function. The only place that it is invoked is withing `lib/blockchain/chain.js` inside of `Chain.connect`. ```bash $ basename $PWD hsd $ grep -rn setBestChain lib lib/blockchain/chain.js:1464: // That will be done outside in setBestChain. lib/blockchain/chain.js:1624: async setBestChain(entry, block, prev, flags) { lib/blockchain/chain.js:2020: await this.setBestChain(entry, block, prev, flags); ``` After doing this review, I believe that it is safe to include the event emitting exactly where it is currently in this pull request. `Chain.setBestChain` is not called for every block, just blocks extending the best chain.

This adds the `"tree commit"` event to `Chain.reconnect` which is part of the codepath for reorgs. This way the event is triggered when reorgs occur. Blocks only ever go through `setBestChain` once, every other time that they are reorged out and then reincluded in the chain, it will be through the `reconnect` method.

Emit an event over websockets for the chain's 'tree commit' event. NodeClient or any websocket client can listen to these events.

boymanjor

I think this is good to go after you handle some commenting nits in the test.

test/node-http-test.js

tynes · 2020-03-27T00:31:21Z

@boymanjor Nits are addressed, thanks for the review. I removed the comments in the test since it reads fairly straightforward.

tynes added api labels Jun 19, 2019

tynes force-pushed the node-event-tree-commit branch from 3b51b24 to 2c61aeb Compare June 21, 2019 05:36

tynes marked this pull request as ready for review June 21, 2019 17:02

tynes added the ready for review label Jun 21, 2019

tynes commented Jun 21, 2019

View reviewed changes

lib/blockchain/chain.js Outdated Show resolved Hide resolved

Mark Tyneway added 3 commits June 21, 2019 11:06

node: emit tree commits over websocket

71a3677

Emit an event over websockets for the chain's 'tree commit' event. NodeClient or any websocket client can listen to these events.

tynes force-pushed the node-event-tree-commit branch 2 times, most recently from 6424f89 to 5a4c491 Compare June 21, 2019 17:12

tynes added ready for review and removed ready for review labels Jun 21, 2019

tynes mentioned this pull request Jun 22, 2019

Wallet websocket covenant events #195

Open

boymanjor approved these changes Mar 26, 2020

View reviewed changes

test/node-http-test.js Outdated Show resolved Hide resolved

test/node-http-test.js Outdated Show resolved Hide resolved

test/node-http-test.js Outdated Show resolved Hide resolved

Mark Tyneway added 2 commits March 26, 2020 17:28

blockchain: setBestChain jsdoc fix

62b60f7

test: node/http websocket tree root

473dbe6

tynes force-pushed the node-event-tree-commit branch from 5a4c491 to 473dbe6 Compare March 27, 2020 00:29

boymanjor merged commit 14e0b83 into handshake-org:master Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node: event tree commit #201

node: event tree commit #201

tynes commented Jun 19, 2019 •

edited

Loading

codecov-io commented Jun 19, 2019 •

edited

Loading

boymanjor commented Jun 19, 2019 •

edited

Loading

tynes commented Jun 21, 2019

tynes commented Jun 21, 2019

chjj commented Jun 21, 2019

tynes commented Jun 21, 2019

boymanjor left a comment

tynes commented Mar 27, 2020

node: event tree commit #201

node: event tree commit #201

Conversation

tynes commented Jun 19, 2019 • edited Loading

codecov-io commented Jun 19, 2019 • edited Loading

Codecov Report

boymanjor commented Jun 19, 2019 • edited Loading

tynes commented Jun 21, 2019

Safety Report

Additional Work

tynes commented Jun 21, 2019

chjj commented Jun 21, 2019

tynes commented Jun 21, 2019

boymanjor left a comment

Choose a reason for hiding this comment

tynes commented Mar 27, 2020

tynes commented Jun 19, 2019 •

edited

Loading

codecov-io commented Jun 19, 2019 •

edited

Loading

boymanjor commented Jun 19, 2019 •

edited

Loading