Cool!
I compiled it against current 0.10 (which is about to be released, modulo a couple of last minute bugfixes), and it compiled fine. I haven't tried running it yet.
Awesome, thanks for looking at it! I'll update it to 0.10 when that's out.
I was surprised to see it doesn't actually use the payment channels API at all, it seems instead to reimplement parts of it. Is there a reason for that? Does the API need some other features for it to be usable by you?
I actually started working on this before 0.9 was published, so a lot of what you see is a mishmash of code based on 0.9-SNAPSHOT that was refactored (but not fully) to 0.10-SNAPSHOT. I had already had a lot of it written (the bottleneck, aside from my limited time due to how busy my paid job and family make me, was testing on testnet before I realized you'd added regtest mode) prior to the BitcoinJ payment channel API. As I was going to release it to the public domain, I figured I wouldn't spend much time looking at the payment channel code in BitcoinJ until I was ready to do so; that would avoid contaminating public domain code with Apache licensed code.
There are a couple of things that would be needed in the BItcoinJ payment channel code to be able to do this:
- The hash preimage part of the SriptPubKey/ScriptSig needs to be supported in order to send payments through chained channels atomically, and is what made the addition of the templates I wrote necessary in bitcoind
- Risk deposits are necessary for the chaining as well, and as far as I can tell, your code doesn't yet support them
- Reversing a payment channel is also useful, and I haven't yet looked to see if your code supports that, though it's probably not too difficult to add
Here are some other tips from a quick review of the code:
Take a read through
the ListenableFuture docs. In many places where in RunTest where you have busy-wait loops, you can replace them with a simpler and cleaner construct.
It's definitely my intent to replace any polling with futures as appropriate, but there are lots of APIs I'm as yet unfamiliar with. This was almost a braindump into code, and I'll clean this part up.
In git master there is a WalletAppKit.connectToLocalHost() method which simplifies some more boilerplate.
I intend to move away from using WalletAppKit and use discrete components instead, but this was good for a starting point. I didn't see you add that method before I wrote the code that connects to the local regtest-mode bitcoind, or I would've used it, thanks!
ScriptBuilder has some static methods that simplify the code you have to create scriptSigs.
I've already started using some of them (in particular, the multisig-related static methods, which I switched away from manually building those scripts opcode-by-opcode), but I'll look at it again and see what else I'm missing. You've been working on ScriptBuilder as I've been working on this, so I probably haven't yet caught up with all the improvements.
This code looks wrong:
this.vChain.addListener(new AbstractBlockChainListener() {
@Override
public void notifyNewBestBlock(StoredBlock block) {
new Thread() {
@Override
public void run() {
for (PaymentChannelGroup peerGroup : connections
.values()) {
peerGroup.checkChannelStates();
}
}
}.start();
}
});
But in 0.10 your listener already runs on a dedicated background thread (the "user thread"). There's no need to create your own dedicated thread for each one. Indeed, this code looks buggy as you could get peerGroup.checkChannelStates() being run in parallel and that method doesn't look thread safe. If you rely on the default behaviour of running on the user thread, your callbacks won't be invoked in parallel so you can just delete the new Thread() {} and get the correct result.
You're right, concurrency is one of the things I haven't yet worked on in this context. In using regtest, I was setting up an environment where this wouldn't matter much, but I need to fix this prior to the code being ready for pre-alpha. The TODO list specifies this. The reason I'm starting a new thread in this context is that when I wrote this code, BitcoinJ was still in 0.9-SNAPSHOT, and I wasn't thinking about thread safety yet.
It's better to use the wallet(), etc accessors if you're going to subclass WalletAppKit.
I'm planning on moving away from that and using discrete components of BitcoinJ as I continue development. There are lots of inconsitencies and things done wrong in my code... I've taken a note on a lot of them in the TODO file, but I have missed some, so thank you for looking over the code! I hope to have time to improve on it in the near future and get it closer to a working model.
Overall though, it's great to see people playing with these ideas and building on bitcoinj. If you reuse the standard payment channels API then you get network connectivity, persistence to the wallet and many other things for free.
Now that I've published this, I'm planning on taking a closer look and seeing if I can use the standard payment channels API instead of what I wrote. Thanks for providing such a great library to build on!
