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

scripts: fuzz: pass hash as buffer not string #900

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pinheadmz
Copy link
Member

One more lingering regression leftover from the great "hashes as buffers instead of strings" refactor from #533

@pinheadmz pinheadmz added the quick Can be fixed quickly, code change less than 10 lines label Nov 1, 2019
@codecov-io
Copy link

Codecov Report

Merging #900 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #900   +/-   ##
=======================================
  Coverage   61.72%   61.72%           
=======================================
  Files         155      155           
  Lines       26058    26058           
=======================================
  Hits        16085    16085           
  Misses       9973     9973

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 77d8804...6346e43. Read the comment docs.

@braydonf
Copy link
Contributor

braydonf commented Nov 1, 2019

Perhaps we should remove (and not maintain) this file? This script is mostly non-functional in general, as the module nodeconsensus is not available anywhere. It was presumably some form of bindings to Bitcoin Core libbitcoinconsensus library similar to bitcoinconsensus. I looked at this a year or so ago, and used a different approach that was compatible with AFL, I can lookup where the branch is if necessary.

@pinheadmz
Copy link
Member Author

Hmm, yeah maybe we should remove the /scripts directory:

certs.sh: only relevant for bip70, removed from bcoin a long time ago
dump.js: requires npm package heapdump, not a bcoin dependency, dumps a heap snapshot in the repo root directory, not sure what its for.
fuzz.js: see above
gen.js: needs a new keyword on line 36, otherwise works. Probably useful when the library was first started, but genesis blocks are stored in lib/protocol/networks.js now.
seeds.sh: pretty cool script to download seed nodes from bitcoin core repository. Had a bug when run on my shell (head: illegal line count -- -1 but that was fixed by changing line 13 & 14 in the script)

@braydonf
Copy link
Contributor

certs.sh: True, only relevant for bip70.
dump.js: Also not sure how it was used.
fuzz.js: Yep, this could become part of fuzz testing setup if maintained.
gen.js: True, also tested the fix and it works, but as mentioned may not be necessary.
seeds.sh: I didn't have a bug when running. This may be useful, seeds will likely need to be updated.

@pinheadmz
Copy link
Member Author

so...

certs.sh: remove
dump.js: remove
fuzz.js: remove? or fix the toString and remove nodeconsensus path?
gen.js: remove
seeds.sh: keep - may be only truly useful script here

...?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quick Can be fixed quickly, code change less than 10 lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants