There seems to be some misunderstandings about this, but it's a heavily used function so I'd like to make sure the docs are clear about how it works.
For a later issue:
utACK I know this is a super old issue but I find the current docs astoundingly confusing, can we try for 15?
@kallewoof I think you have experience with this call?
documentation sounds reasonable, but I don't actually know how it's used.
The way the API is designed to be used is like this:
Let's say I care about only rescanning of 10:
let confsCareAbout = 10
let hash = ""
while (true) {
let res = exec(`bitcoin-cli listsinceblock ${hash} ${confsCareAbout)`)
process(res.transactions)
hash = res.lastblock;
}
It's a pretty sweet work flow for handling deposits. Just unfortunately listsinceblock is too buggy and ill-defined (I'll create some bug reports when I can).
But this PR fixes the docs, that makes it a bit clearer what it does. Most people when they read the docs think it's a filter, when it's not.
1689 | @@ -1690,7 +1690,7 @@ UniValue listsinceblock(const JSONRPCRequest& request) 1690 | "\nGet all transactions in blocks since block [blockhash], or all transactions if omitted\n" 1691 | "\nArguments:\n" 1692 | "1. \"blockhash\" (string, optional) The block hash to list transactions since\n" 1693 | - "2. target_confirmations: (numeric, optional) The confirmations required, must be 1 or more\n" 1694 | + "2. target_confirmations: (numeric, optional, default=1) Return the nth block hash from the main chain. 1 would mean the bestblockhash (see lastblock in return value)\n"
Maybe add a note that transactions with fewer confirmations than this are still included in the response, only lastblock is changed by this argument.
NACK as is
The documentation is very misleading. But it needs to be made even more clear than this. Perhaps add "Transactions with any number of confirmations in the range from blockhash to current tip (or the fork point if blockhash is not on current chain) will be returned." somewhere
(or the fork point if blockhash is not on current chain)
Is that what it actually does? o.0 If so thats ....bizarre and wtf.
Yes, thats what it appears to have always done...docs need to be a shitload clearer here.
I pushed another stab at documenting it based on what @morcos said, but it honestly makes no sense to me. I assume I'm misunderstanding.
Let's imagine I have the blocks:
/--> C
A--->B
\-->D -->E
Where E is the main chain tip. And C is orphaned.
So you're saying if I call listsinceblock C, it's going to return me only transactions between [B, C) AKA transactions I've already processed, and are orphaned?). The expected set of transactions it would return you is between [B, E).
Yes it will return between B and E
Hm, so when you said:
Transactions with any number of confirmations in the range from blockhash to current tip (or the fork point if blockhash is not on current chain) will be returned.
You mean that just the lastblock returns $target_confirmations from the (mainTip or convergencePoint) ?.
But this is independent to the transaction filtering stuff?
1689 | @@ -1690,7 +1690,7 @@ UniValue listsinceblock(const JSONRPCRequest& request) 1690 | "\nGet all transactions in blocks since block [blockhash], or all transactions if omitted\n" 1691 | "\nArguments:\n" 1692 | "1. \"blockhash\" (string, optional) The block hash to list transactions since\n" 1693 | - "2. target_confirmations: (numeric, optional) The confirmations required, must be 1 or more\n" 1694 | + "2. target_confirmations: (numeric, optional, default=1) Return the nth block hash from the main chain. e.g. 1 would mean the bestblockhash, unless [blockhash] is not on the current chain, in which case it would return be the common ancestor block between it an the main chain). Note: this is not used as a filter, but only affects [lastblock] in the return value\n"
No, the value returned is always the Nth-from-top block on the best chain.
Yeah, that's what i thought ><
target_confirmations seems to be handled in a very weird way right now. It only affects the returned last block, and does not actually affect the transactions being returned in any way. I would declare this a bug, personally. The only time the value is used is at https://github.com/bitcoin/bitcoin/blob/0d457c7b274a66d66d59725f5fe1ee1cef5b6391/src/wallet/rpcwallet.cpp#L1859 where
CBlockIndex *pblockLast = chainActive[chainActive.Height() + 1 - target_confirms];
i.e. to determine the block hash of he last block.
Correct me if I'm wrong.
@kallewoof you are correct, but that behavior long predates the documentation for the function. I have no idea if anyone uses its previous behavior, but assuming people test things before putting them in production, best to fix the docs and leave the behavior IMO.
@TheBlueMatt That makes sense to me.
I don't think the behavior is a bug at all, it was intentionally designed for use in a loop like I describe in an earlier comment.
It's actually a really nice and simple way to handle deposits, and I know a few people who do it that way. Note how it works really well if your depositor goes down, because you keep storing the hash you want to scan from. Changing the behavior at this point would be totally unreasonable.
(The only reason I don't handle deposits myself that way, is how it handles double spend (attempts) are weird and inconsistent, which make it very difficult to work with when you're showing unconfirmed stuff)
ACK 52295ba
I think that language is clear and accurate
(except I'm not sure about putting brackets around the argument names in the help text, is that something we do anywhere else?)
(except I'm not sure about putting brackets around the argument names in the help text, is that something we do anywhere else?)
I just copied it from the original. It's a bit messy, but it's pretty clear and makes things a lot less ambiguous.
(The only reason I don't handle deposits myself that way, is how it handles double spend (attempts) are weird and inconsistent, which make it very difficult to work with when you're showing unconfirmed stuff)
The reason why double spends are weird and inconsistent is probably because you are using target_confs > 1. This means if there's a 1-2 block fork, and your target_confs = 10 or something, you will not be told of the differences caused by the orphan since you're listing a block that preceded the fork point.
That's how I interpret this, anyway.
My initial expectation was that transactions with less than target_confs confirmations (i.e. transactions in blocks newer than the returned lastblock at the end) would be excluded from the results. This would give you a buffer of 0-confs and 1-confs that are still at risk. That's not what is happening though.
1717 | @@ -1717,7 +1718,7 @@ UniValue listsinceblock(const JSONRPCRequest& request) 1718 | " \"label\" : \"label\" (string) A comment for the address/transaction, if any\n" 1719 | " \"to\": \"...\", (string) If a comment to is associated with the transaction.\n" 1720 | " ],\n" 1721 | - " \"lastblock\": \"lastblockhash\" (string) The hash of the last block\n" 1722 | + " \"lastblock\": \"lastblockhash\" (string) The hash of the block (target_confirmations-1) from the best block on the main chain. This is typically used to feed back into listsinceblock the next time you call it. So you would generally use a target_confirmations of say 6, so you can keep rescanning the last 6 blocks plus any new ones\n"
"rescanning" means something specific, maybe "so you will be continually re-notified of transactions until they've reached 6 confirmations"
Needs rebase and addressing of @TheBlueMatt 's nit, if this is still to make 0.15
Ok, rebased and addressed Matt's nit
1754 | @@ -1755,14 +1755,13 @@ UniValue listsinceblock(const JSONRPCRequest& request) 1755 | 1756 | if (request.fHelp || request.params.size() > 4) 1757 | throw std::runtime_error( 1758 | - "listsinceblock ( \"blockhash\" target_confirmations include_watchonly include_removed )\n" 1759 | - "\nGet all transactions in blocks since block [blockhash], or all transactions if omitted.\n" 1760 | - "If \"blockhash\" is no longer a part of the main chain, transactions from the fork point onward are included.\n" 1761 | - "Additionally, if include_removed is set, transactions affecting the wallet which were removed are returned in the \"removed\" array.\n" 1762 | + "listsinceblock ( \"blockhash\" target_confirmations include_watchonly)\n"
Looks like a rebase error - you've removed the include_removed parameter here.
Sorry, yeah. I fail. Pushed a fix.
1754 | @@ -1755,14 +1755,13 @@ UniValue listsinceblock(const JSONRPCRequest& request) 1755 | 1756 | if (request.fHelp || request.params.size() > 4) 1757 | throw std::runtime_error( 1758 | - "listsinceblock ( \"blockhash\" target_confirmations include_watchonly include_removed )\n" 1759 | - "\nGet all transactions in blocks since block [blockhash], or all transactions if omitted.\n" 1760 | - "If \"blockhash\" is no longer a part of the main chain, transactions from the fork point onward are included.\n" 1761 | - "Additionally, if include_removed is set, transactions affecting the wallet which were removed are returned in the \"removed\" array.\n"
Rebase error here, too.
1765 | "\nArguments:\n" 1766 | "1. \"blockhash\" (string, optional) The block hash to list transactions since\n" 1767 | - "2. target_confirmations: (numeric, optional, default=1) The confirmations required, must be 1 or more\n" 1768 | - "3. include_watchonly: (bool, optional, default=false) Include transactions to watch-only addresses (see 'importaddress')\n" 1769 | + "2. target_confirmations: (numeric, optional, default=1) Return the nth block hash from the main chain. e.g. 1 would mean the bestblockhash. Note: this is not used as a filter, but only affects [lastblock] in the return value\n" 1770 | + "3. include_watchonly: (bool, optional, default=false) Include transactions to watch-only addresses (see 'importaddress')"
Why did you delete the \n here?
some how screwed that up in the rebase. fixed
Looks like a few rebase errors.
1763 | + "If [blockhash] is not in the best chain, it will return all transactions since [blockhash]'s first common ancestor with the best chain\n" 1764 | "Additionally, if include_removed is set, transactions affecting the wallet which were removed are returned in the \"removed\" array.\n" 1765 | "\nArguments:\n" 1766 | "1. \"blockhash\" (string, optional) The block hash to list transactions since\n" 1767 | - "2. target_confirmations: (numeric, optional, default=1) The confirmations required, must be 1 or more\n" 1768 | + "2. target_confirmations: (numeric, optional, default=1) Return the nth block hash from the main chain. e.g. 1 would mean the bestblockhash. Note: this is not used as a filter, but only affects [lastblock] in the return value\n"
I dont think "bestblockhash" is one word.
Ok. ðŸ˜
Ok, fixed my rebase fail...
utACK.
utACK
utACK 9f8a46f