search

Start9Labs / bitcoind-startos

wrapper for building bitcoind.s9pk
Other
14 stars 20 forks source link

Add Bloom Filters #68

Closed kn0wmad closed 2 years ago

kn0wmad commented 2 years ago

Successfully tested a connection with Bisq

Closes #67

chrisguida commented 2 years ago

Also you should add a warning about this being discouraged for everything except bisq

kn0wmad commented 2 years ago

This change, on its own/in its current form, will break bitcoind. You need to write a migration for the config spec since you changed the structure of how the config.yaml is organized. Otherwise this looks good

Added the parameter to the lt_22_0_0 script with the other filters - will test migration

chrisguida commented 2 years ago

@ProofOfKeags I think the only change that needs to be made here is that 22_0_0 needs to be renamed 22_0_2 and referenced under that version in the manifest. But hopefully any issues will be uncovered when @kn0wmad tests the migration

ProofOfKeags commented 2 years ago

@ProofOfKeags I think the only change that needs to be made here is that 22_0_0 needs to be renamed 22_0_2 and referenced under that version in the manifest. But hopefully any issues will be uncovered when @kn0wmad tests the migration

This is insufficient as it is currently written it is trying to mutate filters which doesn't exist in the current deployment. The migration needs to move the contents of blockfilters into filters

kn0wmad commented 2 years ago

@ProofOfKeags I think the only change that needs to be made here is that 22_0_0 needs to be renamed 22_0_2 and referenced under that version in the manifest. But hopefully any issues will be uncovered when @kn0wmad tests the migration

This is insufficient as it is currently written it is trying to mutate filters which doesn't exist in the current deployment. The migration needs to move the contents of blockfilters into filters

I see... maybe it would make more sense to keep the name "Block Filters" - I wasn't sure if it was an accurate description or not to include bloom filters there, but it can also be in it's own category

chrisguida commented 2 years ago

@ProofOfKeags it's actually adding new keys with the new names, and defaulting them to false. But you're right that it needs to move the old keys to the new names rather than simply creating the new keys and defaulting them to false.

@kn0wmad there are probably other things you need to check as well, like whether the stuff left over from 22.0.0 can be removed, or whether it needs to be separated into its own migration. This is the first time we're going to have two migrations in a package. You need to test the hell out of this.

kn0wmad commented 2 years ago

Deprecating this PR in favor of #69, inshallah