Fix: updated the sync progress function

[masterchief164]

Closes #886

We now calculate the progress according to the number of blocks mined until 17:36:38 UTC on 3rd December 2022 and then extrapolate the number of blocks assuming the 144 blocks are mined every day. This PR fixes issue #886.

[codecov-commenter]

Codecov Report

Base: 69.36% // Head: 69.34% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (e4ae2ad) compared to base (69d689a).
Patch coverage: 93.33% of modified lines in pull request are covered.

❗ Current head e4ae2ad differs from pull request most recent head b4e4934. Consider uploading reports for the commit b4e4934 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1110      +/-   ##
==========================================
- Coverage   69.36%   69.34%   -0.02%     
==========================================
  Files         158      158              
  Lines       26590    26601      +11     
==========================================
+ Hits        18443    18446       +3     
- Misses       8147     8155       +8     

Impacted Files	Coverage Δ
lib/blockchain/chain.js	72.65% <90.00%> (+0.06%)	⬆️
lib/protocol/network.js	83.72% <100.00%> (+0.12%)	⬆️
lib/protocol/networks.js	100.00% <100.00%> (ø)
lib/golomb/reader.js	77.35% <0.00%> (-11.33%)	⬇️
lib/net/hostlist.js	50.00% <0.00%> (-0.16%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pinheadmz]

This patch doesn't actually solve the issue as described in #886. What we need to do is base progress estimation not on blocks or time (which are consistent throughout the life of the Bitcoin blockchain) but on TRANSACTIONS (which are not).

For example, you could process 50% of the blockchain (2009-2015 or so) very quickly because during that era TX activity was relatively low. Once your node gets to ~2016 and on, the blocks get processed much slower because they have so many more transactions.

That's the cool trick that Bitcoin Core does - it actually gives you a % progress not on the number of blocks but more accurately on how much actual UTXO/hashing/ECDSA work is left for your specific machine.

So the goal would be an initial block download where the progress advances at the same rate the whole time from 0-100%

[pinheadmz]

Here's the results of one sync test I ran. The x axis is whole number progress % reported in the log and the y axis is seconds between those whole number progress %s. So for example we'd expect the time between 40% and 41% to be the same amount of time between 89% and 90%, etc.

[pinheadmz]

Latest commit seems to match bitcoin core pretty well

bcoin on the left, bitcoind on the right:

[masterchief164]

LGTM!

[pinheadmz]

merged in 57f5956

[bolaum]

@masterchief164 @pinheadmz
Hi! Should this new progress solution work with SPV nodes? I tried to sync a spv node for testnet and progress is showing 0% even after syncing half the blocks. Should we keep the old code for SPV nodes?

[pinheadmz]

@bolaum great catch, yeah we did not consider SPV mode! I think chain.db.state.tx will always remain at 0 for SPV nodes. Can reproduce the behavior in chain-progress-test.js:

diff --git a/test/chain-progress-test.js b/test/chain-progress-test.js
index 456267105..b03b38618 100644
--- a/test/chain-progress-test.js
+++ b/test/chain-progress-test.js
@@ -154,7 +154,8 @@ describe('Chain Sync Progress', function () {
     const newChain = new Chain({
       memory: true,
       blocks,
-      network
+      network,
+      spv: true
     });
 
     before(async () => {

[bolaum]

I did this in my branch:

diff --git a/lib/blockchain/chain.js b/lib/blockchain/chain.js
index 1c252b81..9cd0a312 100644
--- a/lib/blockchain/chain.js
+++ b/lib/blockchain/chain.js
@@ -2035,6 +2035,13 @@ class Chain extends AsyncEmitter {
    */
 
   getProgress() {
+    if (this.options.spv) {
+      const start = this.network.genesis.time;
+      const current = this.tip.time - start;
+      const end = util.now() - start - 40 * 60;
+      return Math.min(1, current / end);
+    }
+
     const time = util.now();
 
     const currentCount = this.db.state.tx;

Is this an acceptable PR? 😬

[pinheadmz]

Is this an acceptable PR? 😬

Yes but try to add a test, or include SPV in the existing chain-progress-test.js file. It might be possible to indent the entire describe block after wrapping it in a for loop (see example diff below). This will run the entire test twice, once with an SPV chain and once with a Full Node chain. Right now, this test fails. I expect that in SPV mode the actual progress tested at each stage will be different, so it might need some extra if (spv) logic throughout:

diff --git a/test/chain-progress-test.js b/test/chain-progress-test.js
index 456267105..43a353d46 100644
--- a/test/chain-progress-test.js
+++ b/test/chain-progress-test.js
@@ -140,7 +140,8 @@ describe('Chain Sync Progress', function () {
     assert.strictEqual(chain.db.state.tx, (1 + 100 + 200 + 300));
   });
 
-  describe('New chain', function () {
+for (const spv of [true, false]) {
+  describe(`New chain: ${spv ? 'SPV' : 'Full'}`, function () {
     // time never changes
     util.now = () => {
       return chain.tip.time;
@@ -154,7 +155,8 @@ describe('Chain Sync Progress', function () {
     const newChain = new Chain({
       memory: true,
       blocks,
-      network
+      network,
+      spv
     });
 
     before(async () => {
@@ -222,4 +224,5 @@ describe('Chain Sync Progress', function () {
       assert.strictEqual(percent, 100);
     });
   });
+}
 });

[bolaum]

@pinheadmz PR: #1137
Thanks!