-
Notifications
You must be signed in to change notification settings - Fork 68
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
blog: add 2024 code audit #206
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for awesome-golick-685c88 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
7d8a45b
to
ddf96aa
Compare
I also added a copy of the original audit github gist files. If some day the github version goes away we can link to our own static copy. |
ddf96aa
to
1229348
Compare
Awesome post! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK.
draft: false | ||
--- | ||
|
||
A heartfelt thank you to our friends at [Spiral](https://spiral.xyz/) for sponsoring a code audit of the current `bdk` 1.0.0-beta Rust codebase. The effort was led by [Antoine Poinsot](https://gist.github.com/darosior) from [Wizardsardine](https://wizardsardine.com/), who did a fantastic job provided insightful and actionable recommendations for the BDK team. You can find the full report [here](https://gist.github.com/darosior/4aeb9512d7f1ac7666abc317d6f9453b). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...who did a fantastic job provided... ---->
...who did a fantastic job providing...?
crash bug which can be triggered by a third party, or a significant slowdown in performing some | ||
operations). | ||
|
||
A threat model i considered interesting for the purpose of this review was that of a server-side |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't I
always be capitalized?
and consisted of manually testing the library, looking for surprising behaviour in corner cases. | ||
Finally, i wrote two fuzz targets (code is shared along with this report): | ||
- The first one exercised the `Wallet` interface under a number of conditions with data generated by | ||
the fuzzer (apply an update, roundtrip to persistence, or create a transaction -then repeat). It's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or create a transaction -then repeat
--> or create a transaction, then repeat
|
||
I haven't thought about a realistic scenario in which this could be an issue, but note two different | ||
descriptor templates (as in actual descriptors are differing, not only the xpubs inside) can have | ||
the same descriptor ID. For instance `multi(1,A,B)` and `sortedmulti(1,B,A)`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: double space between ...descriptor ID
. and For instance...
Only the 4 modules i focused on already amount to 15,777 lines of code (without accounting for the | ||
unit test directories). Here is a list of things i did not have time to do and think would be | ||
beneficial: | ||
- More fuzz coverage. I only started writing a couple fuzz targets after having reviewed the 15k |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we open a new issue considering the implementation of more fuzz targets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
draft: false | ||
--- | ||
|
||
A heartfelt thank you to our friends at [Spiral](https://spiral.xyz/) for sponsoring a code audit of the current `bdk` 1.0.0-beta Rust codebase. The effort was led by [Antoine Poinsot](https://gist.github.com/darosior) from [Wizardsardine](https://wizardsardine.com/), who did a fantastic job provided insightful and actionable recommendations for the BDK team. You can find the full report [here](https://gist.github.com/darosior/4aeb9512d7f1ac7666abc317d6f9453b). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: possibly use Antoines GitHub Profile https://github.com/darosior
instead of Gist Profile https://gist.github.com/darosior
(even though I understand we link to his gist of the full report)? Just a suggestion though, either way totally works.
The effort was led by [Antoine Poinsot](https://github.com/darosior)
|
||
While no critical defects were identified, a potential denial of service/performance issue was uncovered, along with opportunities to improve the code's fault tolerance and API documentation. The team is currently addressing the performance issue, as well as some of the more straightforward recommendations. All suggested improvements have been [added to our issues backlog](https://github.com/bitcoindevkit/bdk/issues?q=is%3Aissue+label%3Aaudit) for future releases. | ||
|
||
If you are a user or potential user of BDK, or a Bitcoin Rust developer, we would love to hear your feedback. Please reach out on the [BDK Discord](https://discord.gg/dstn4dQ) or comment on individual [Github issues](https://github.com/bitcoindevkit/bdk/issues?q=is%3Aissue+is%3Aopen). As a fully free and open-source project, the BDK team relies on YOU our community of users and contributors to help us deliver the best Bitcoin wallet library possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: change Github
to GitHub
for this or comment on individual [Github issues]
No description provided.