Skip to content
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

Adding Prune command #69

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

StefanoFedeli
Copy link

First draft of the prune command asked in here, right now the command is pretty poor:

  • No options available
  • Prune if there is at least one bitcoin attestation, deleting all others attestations in the timestamp
  • No verification of the attestations before the prune.

The branch is not meant to be merged as it is but as a starting point to add the new feature.

@lvaccaro
Copy link
Member

lvaccaro commented Jan 4, 2020

Hi @StefanoFedeli really useful, thank you.
It looks quite good, I'll add some feedbacks on the code.

Could you explicit the used strategy to choose which attestations to remove?

* @param {DetachedTimestampFile[]} detaches - The array of detached file to stamp; input/output parameter.
* @param {Object} options - The option arguments. Not used yet
*/
prune (detaches, options = {}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to update the prune prototype in order to take one DetachedTimestampFile at time, instead an array of file (following the PR's code you are just handle 1 file at time).

@@ -1,6 +1,7 @@
const test = require('tape')
const Calendar = require('../src/calendar.js')
const Utils = require('../src/utils.js')
const { URL } = require('url')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is auto merging stuff that add URL here and in the src/calendar.js file

* Delete all the leaves which didn't match the height passed
*/
discard_attestation (timestamp, heightToKeep) {
timestamp.getAttestations().forEach(opStamp => {
Copy link
Member

@lvaccaro lvaccaro Jan 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getAttestations() returns a Set of every attestations walking the timestamp tree until the leaf. I think in this case, you need to check only the current attestations as: timestamp.attestations

let prunable = false
let maxHead = 0

if (detaches.timestamp.getAttestations().length <= 1) {
Copy link
Member

@lvaccaro lvaccaro Jan 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

count only BitcoinBlockHeaderAttestation, because usually there is at least one PendingAttestation that shouldn't be counted

var iter = timestamp.ops.values()
let res = iter.next()
while (!res.done) {
this.discard_attestation(res.value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add the heightToKeep param

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants