Add blockchain.outpoint.subscribe RPC #454
Conversation
|
Code looks very good to me (much better than mine in #446 obviously) and I successfully use it with janoside/btc-rpc-explorer#356 This "test" doesn't use the notification part however ! |
romanz
force-pushed
the
p2p-outpoint-rpc
branch
from
a1d5efa
to
5d2c7cf
Compare
|
Thanks for testing! |
romanz
force-pushed
the
p2p-outpoint-rpc
branch
from
5d2c7cf
to
ea5a907
Compare
|
Would it be possible to rebase this PR with current p2p branch ? I would like to try #468 with my PR in the bitcoin explorer janoside/btc-rpc-explorer#356 |
romanz
force-pushed
the
p2p-outpoint-rpc
branch
from
ea5a907
to
aec27af
Compare
|
Thanks for the heads-up! |
romanz
force-pushed
the
p2p-outpoint-rpc
branch
2 times, most recently
from
eae0905
to
7b728d4
Compare
romanz
force-pushed
the
p2p-outpoint-rpc
branch
3 times, most recently
from
78e62ab
to
30f7401
Compare
|
I suppose you will reopen this PR once Electrum 1.5 spec are merged ? Great work on the |
|
I think this was the same GitHub confusion as in #455 |
Yes, reopening now :) |
romanz
force-pushed
the
p2p-outpoint-rpc
branch
2 times, most recently
from
cd42567
to
f9239e0
Compare
romanz
force-pushed
the
p2p-outpoint-rpc
branch
from
f9239e0
to
919c660
Compare
romanz
force-pushed
the
p2p-outpoint-rpc
branch
2 times, most recently
from
7db0b8c
to
049df10
Compare
romanz
force-pushed
the
p2p-outpoint-rpc
branch
2 times, most recently
from
08903b6
to
c9cfcf5
Compare
romanz
force-pushed
the
p2p-outpoint-rpc
branch
2 times, most recently
from
8fe9fff
to
70025e4
Compare
romanz
force-pushed
the
p2p-outpoint-rpc
branch
2 times, most recently
from
91b01b5
to
00faaae
Compare
romanz
force-pushed
the
p2p-outpoint-rpc
branch
from
00faaae
to
af7b889
Compare
romanz
force-pushed
the
p2p-outpoint-rpc
branch
from
af7b889
to
edd1a30
Compare
|
Hi ! Just for information: This PR still work as expected when cherry picked from master. This is mandatory since #783 The only thing that must be changed to make it compiling is BlockHash::default() to BlockHash::all_zeros() in OutpointStatus struct. I re tACK this with janoside/btc-rpc-explorer/pull/356 |
romanz
force-pushed
the
p2p-outpoint-rpc
branch
2 times, most recently
from
45a13d1
to
e0d42ac
Compare
| for tx in block.txdata { | ||
| let txid = tx.txid(); | ||
| let output_len = u32::try_from(tx.output.len()).unwrap(); | ||
| if self.outpoint.txid == txid && self.outpoint.vout < output_len { |
There was a problem hiding this comment.
self.outpoint.vout < output_len
maybe i am missing something, but for what reason is this check here?
There was a problem hiding this comment.
Clearly this avoids out-of-bounds access.
There was a problem hiding this comment.
hmm i tried to find where self.outpoint.vout is used to index tx.output, but could not find it, self.outpoint.vout is not used after this here, which made sense after i thought of this: this code checks for output funding, it doesnt matter which output index is subscribed to, as long as it is in the transaction (self.outpoint.vout < output_len)
There was a problem hiding this comment.
Oh, interesting, this is not even required for double-spend handling since a double-spent transaction would have a different txid.
Maybe there would be a value in having a sanity check with a warning? It'd be best to also check scripts then.
There was a problem hiding this comment.
It's indeed a sanity check - in case the user tries to subscribe to a non-existing outpoint (by specifying a vout larger than the actual # of txid outputs).
There was a problem hiding this comment.
What I'm trying to say is that it now silently ignores the transaction instead of printing a warning and a warning would be helpful because doing this is a bug and without a message it could be hard to analyze.
romanz
force-pushed
the
p2p-outpoint-rpc
branch
2 times, most recently
from
c77b4d6
to
e11fff1
Compare
Following #444 (see the specification here).
Blocked until Electrum client is released with support of 1.5 protocol.