Conversation
* Loading taproot configuration + correct configurations for Alice, Bob and Charlie * more robust
|
After rebasing I started to have this error : I am not sure how to fix it. |
Try running |
|
After adding the mocked power it seems that the error has disappeared. |
* fail to detect actor code * working but need clean * remove println
* saving file /tmp * verify if share file exist
chain/checkpointing/sub.go
Outdated
| id := party.ID(c.host.ID().String()) | ||
|
|
||
| threshold := 2 | ||
| n := NewNetwork(ids, c.sub, c.topic) |
There was a problem hiding this comment.
NewNetwork is independent in every round of the DKG and that is why we start it every time a new state change is detected? If this is the case we should probably try to find a better name for the Network struct :) From my first read, it felt like that is the permanent network layer used by the DKG protocol (and reused in every round)
| @@ -0,0 +1,185 @@ | |||
| [API] | |||
There was a problem hiding this comment.
Just noting it here for the future. Before merging and getting this ready for production, we should probably move these configs to a scripts/taproot that we can use for testing purposes, or remove them. Also, we can probably remove all the commented lines if they are already in the config template.
There was a problem hiding this comment.
We agreed to move everything that is under data will be move in Minio and downloaded before starting the demo. Those are pre-generated keys and config only required when developing/running the demo.
| if newTs.Height()%20 == 0 { | ||
| fmt.Println("Check point time") | ||
|
|
||
| // Initiation and config should be happening at start |
There was a problem hiding this comment.
We may need to revise this logic to be sure that:
- The checkpoint is running every 20 blocks.
- We may need to wait a few epochs before detecting a checkpointing epoch to ensure finality (the tipset we detect in the event change may be reverted and not be final, and committing a reverted tipset in a checkpoint may be quite bad).
- We should check that we are only committing the checkpoints one. Due to the way
checkFuncworks, it may trigger theChangeFuncevery time the state change is detected, which could end up translating in a checkpoint being committed several times.
Just wanted to note it here so we test it at some point.
There was a problem hiding this comment.
Also, we should probably add a config parameter to configure the checkpointing period.
There was a problem hiding this comment.
Re-reading the function again, it may be a good idea to detect the change here, wait for a few epochs, and the trigger the changeFunc to create a new checkpoint. That way we ensure finality. I do something similar here in case you need inspiration:
chain/checkpointing/sub.go
Outdated
| */ | ||
|
|
||
| f := frost.SignTaprootWithTweak(c.config, ids, hashedTx[:], c.tweakedValue[:]) | ||
| n := NewNetwork(ids, c.sub, c.topic) |
There was a problem hiding this comment.
We should definitely the change the name of the Network struct, because without additional docs it is very misleading.
There was a problem hiding this comment.
I will add a comment. Any name suggestions ? I re-use Network because it is how it was called in the lib and our script.
There was a problem hiding this comment.
NewRound maybe? Or something like NewState? Let's try to find something more self-describing than Network if we can. It led me to confusion the first time I read it :)
There was a problem hiding this comment.
I thought of NewRound too but it could be confusing at the cryptographic level because there is several rounds of communication when you do the DKG/signing... What about NewNetInterface ? Because it is an interface for communication between participants. They can receive and send messages through this instance.
What about Communication ? NewCommunicationRound ?
|
|
||
| lc.Append(fx.Hook{ | ||
| OnStop: func(ctx context.Context) error { | ||
| // Do we need to stop something here ? |
There was a problem hiding this comment.
Probably not much. In Start you are joining the keygen topic, you should probably leave it here,but I can't think of anything else that needs to be done right now.
chain/checkpointing/sub.go
Outdated
| func (c *CheckpointingSub) prefundTaproot() error { | ||
| taprootAddress := PubkeyToTapprootAddress(c.pubkey) | ||
|
|
||
| payload := "{\"jsonrpc\": \"1.0\", \"id\":\"wow\", \"method\": \"sendtoaddress\", \"params\": [\"" + taprootAddress + "\", 50]}" |
There was a problem hiding this comment.
Same from above regarding raw payload v.s. marshalled struct.
| } | ||
| newValue := value - FEE | ||
|
|
||
| payload := "{\"jsonrpc\": \"1.0\", \"id\":\"wow\", \"method\": \"createrawtransaction\", \"params\": [[{\"txid\":\"" + c.ptxid + "\",\"vout\": " + strconv.Itoa(index) + ", \"sequence\": 4294967295}], [{\"" + newTaprootAddress + "\": \"" + fmt.Sprintf("%.2f", newValue) + "\"}, {\"data\": \"" + hex.EncodeToString(data) + "\"}]]}" |
There was a problem hiding this comment.
Quick question: why are you defining the raw payload instead of creating a struct and then marshalling? It may be more readable and less prone to bugs.
| return h.Sum(nil) | ||
| } | ||
|
|
||
| func TaprootSignatureHash(tx []byte, utxo []byte, hash_type byte) ([]byte, error) { |
There was a problem hiding this comment.
@sa8 may I ask for you for some help reviewing this part? I am not super familiar with our design and I may be missing some details. Happy to do it sync together if it works best for you (that way I can learn a bit more about how the protocol works :) ).
There was a problem hiding this comment.
@sa8 Here the BIP 341 detailing how a message should be serialized before being signed (https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#common-signature-message). The scheme for Taproot has changed and is a bit more complicated.
You can also look at this python script used to test against the golang function (https://github.com/Zondax/fil-taproot/blob/main/scripts/main.py).
This step is being done right before signing and it return a hash. The hash is the message then signed by the participants.
chain/checkpointing/util.go
Outdated
| return taprootAddressLegacy | ||
| } | ||
|
|
||
| func ApplyTweakToPublicKeyTaproot(public []byte, tweak []byte) []byte { |
There was a problem hiding this comment.
@sa8 same for the next few function related with taproot if possible. Thanks :)
|
@rllola, I did a first pass through the code and left a few comments. Let me know if you feel any part of the code needs a deeper review at this point. Feel free to resolve the comments as you fix them or if you see that they don't apply, and ping me whenever you need/want another review. |
|
|
||
| // If Power Actors list has changed start DKG | ||
| // Changes detected so generate new key | ||
| if oldSt.MinerCount != newSt.MinerCount { |
There was a problem hiding this comment.
we may only need to check that the size of symmetrical difference between the two sets is bigger than some threshold u.
| } | ||
|
|
||
| func GetTaprootScript(pubkey []byte) string { | ||
| return "5120" + hex.EncodeToString(pubkey) |
There was a problem hiding this comment.
It is the taproot script. The taproot script should start with 1 and is followed by the pubkey.
| return false, nil, nil | ||
| } | ||
|
|
||
| err := c.events.StateChanged(checkFunc, changeHandler, revertHandler, 5, 76587687658765876, match) |
There was a problem hiding this comment.
This is the confidence threshold used to determine if the StateChangeHandler should be triggered. It is an absurdly high number so the metric used to determine if to trigger it or not is the number of tipsets that have passed in the heaviest chain (the 5 you see there)
* increasing the message size for libp2p * added comments and logs * removing deprecated functions * improve logics by removing some init steps outside of eudico
3 participants
Abstract
This PR add the checkpointing feature in Bitcoin (see #41).
Details
config.toml(see./data/alice/config.toml)Run it
You will need docker bitcoind running locally under port
18443with rpc usersatoshiand rpc passwordamiens.Under
datayou will find preconfigure participants in order to quickly start (Alice, Bob and Charlie).It will start 3 nodes and a miner.