-
Notifications
You must be signed in to change notification settings - Fork 186
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
Add Browser Support #295
Add Browser Support #295
Conversation
bbolt has a heavy dependency on memory mapping and the file system which isn't yet supported in the browser wasm runtime, so instead tempdb keeps everything in-memory so we can support both browser and native targets while testing.
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.
Nice work! This is super exciting, we have a number of other projects in the background that stand to tremendously benefit from this line of work.
Did an initial round of review, will also check out the source of the indexeddb
wrapper you created so I can get a handle on what type of concurrency control we can count on.
db, err := walletdb.Create( | ||
"bdb", tempDir+"/weks.db", true, dbOpenTimeout, | ||
) | ||
db, err := walletdb.Create("tempdb", "weks.db") |
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.
Perhaps we can instead modify the tests to use a build tag to specify tempdb
vs bdb
?
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.
We could have a NewTestDB
func defined for the build w/ and w/o the build tag defined.
@@ -173,12 +173,18 @@ func TestHeaderStorageFallback(t *testing.T) { | |||
t.Fatalf("error setting new tip: %v", err) | |||
} | |||
for _, header := range newHeaderEntries { | |||
if err := hIndex.truncateIndex(&header.hash, true); err != nil { | |||
// Copy the header hash so we can create a iteration-specific pointer. | |||
// https://github.com/golang/go/discussions/56010#discussion-4441437. |
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.
If we bump the Go version to 1.22 here, then we can bind the new behavior directly.
// be found. | ||
type ErrHeaderNotFound struct { | ||
error | ||
// headerBufPool is a pool of bytes.Buffer that will be re-used by the various |
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.
I think this would be easier to review if one commit moved the contents into this new file, then subsequent commits start to add the build tag and additional refactoring.
// writing headers to disk. | ||
var headerBufPool = sync.Pool{ | ||
New: func() interface{} { return new(bytes.Buffer) }, | ||
// Close and delete the BlockHeaderStore. |
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.
godoc
comment should start with the method name
// "header" as it deals only with raw bytes, and leaves it to a higher layer to | ||
// interpret those raw bytes accordingly. | ||
// | ||
// TODO(roasbeef): quickcheck coverage. |
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.
stray todo/copy pasta
type headerStore struct { | ||
mtx sync.RWMutex // nolint:structcheck // false positive because used as embedded struct only | ||
|
||
idb *indexeddb.DB |
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.
What if we had this package implement the walletdb
interface? Then we could just drop it in here, as well as some other locations/projects that use that central KV-store abstraction.
hdrs := tx.Store(headersStore) | ||
|
||
// Count the amount of headers. | ||
count, err := hdrs.Count() |
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.
Under the hood, does this hit a fast index, or will it need to enumerate all the contents to get a final sum?
// headerStore combines a IndexedDB store of headers within a flat file in addition | ||
// to a database which indexes that flat file. Together, these two abstractions | ||
// can be used in order to build an indexed header store for any type of | ||
// "header" as it deals only with raw bytes, and leaves it to a higher layer to |
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.
I think we should revise this godoc
a bit to note the high level structure of this impl. IIUC, it maps height => header
within indexeddb
. It's the based off a similar bucket structure as boltdb, with the top level key here being headersStore
.
} | ||
|
||
// Delete the header. | ||
return hdrs.Delete(height + 1) |
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.
not zero indexed?
|
||
func (h *headerStore) singleTruncate() error { | ||
// Get the current height. | ||
height, genesis, err := h.height() |
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.
Need to check out the underlying implementation, but: should the height be obtained after the transaction is created? Otherwise, two writers may start the transaction with an inconsistent view.
@@ -1,18 +1,211 @@ | |||
//go:build !windows && !js && !wasm |
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.
Why is windows excluded?
fhs := &FilterHeaderStore{ | ||
fStore, | ||
bhs := &blockHeaderStore{ | ||
headerStore: hStore, |
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.
Do you think we can make this headerStore field an interface? since we want to interact with the methods only and there are currently two implementations for this. Each implementation smartly separated by your use of build tags.
// determined by the hight value. | ||
func (h *headerStore) readHeader(height uint32) (wire.BlockHeader, error) { | ||
// Create a new read/write transaction, scoped to headers. | ||
tx, err := h.idb.NewTransaction([]string{headersStore}, indexeddb.ReadWriteMode) |
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.
Do you think this should transaction should be created in ReadMode only?
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.
Thanks for the PR. I left some comments and suggestions. The commit title should be:
package name: Message.
@linden, remember to re-request review from reviewers when ready |
Closing due to inactivity |
9 similar comments
Closing due to inactivity |
Closing due to inactivity |
Closing due to inactivity |
Closing due to inactivity |
Closing due to inactivity |
Closing due to inactivity |
Closing due to inactivity |
Closing due to inactivity |
Closing due to inactivity |
!lightninglabs-deploy mute |
closing this PR for now, Go's lack of multi-threading in browser wasm has made a large amount of we plan to pick this back up as soon as Go and any mainstream browser supports this. |
This PR adds support for the browser using indexeddb as a storage backend for
headerfs
instead of the file system.Why IndexedDB?
I'd initially chosen to go with localStorage for simplicity but due to recent rate limiting changes it made syncing past a couple 100 blocks freeze, indexddb does not have share these limitations. I'd also looked into emulating the file system but it turned out to add a lot of complexity with a performance loss over just using indexeddb directly.
Browser Testing.
Since Go only provides an executor for your native platform testing needs to be done via a custom executor targeting the browser. I've written my own called
wasmexec
which also provides file system access for ease of testing, you can run tests via the following:I'm planning on provider this as part of a Github Action in the future.
btcd
Changes.Running this in the browser without
wasmexec
's file system access requires a couple of changes tobtcd
'saddrmgr
&btcutil
as they require a file system. Changes made in btcsuite/btcd#2124.