[bitcoin-dev] The new obfuscation patch & GetStats

[Daniel Kraft via bitcoin-dev]

Hi!

I hope this is not a stupid question, but I thought I'd ask here first
instead of opening a Github ticket (in case I'm wrong).

With the recently merged "obfuscation" patch, content of the
"chainstate" LevelDB is obfuscated by XOR'ing against a random "key".
This is handled by CLevelDBWrapper's Read/Write methods, which probably
cover most of the usecases.

*However*, shouldn't it also be handled when iterating over the
database? In particular, I would expect that the obfuscation key is
applied before line 119 in txdb.cpp (i. e., while iterating over the
coin database in CCoinsViewDB::GetStats).

Is there a reason why this need not be done there, or is this an actual
oversight?

Yours,
Daniel

[James O'Beirne via bitcoin-dev]

Hey, Daniel.

Patch author here. Thanks for the diligence; I think this indeed may be an
oversight, though I'm going to need to look into a bit more thoroughly at
home. Curious that it didn't fail any of the automated tests.

Correct me if I'm wrong, but the only actual invocation of that method is
here
<https://github.com/bitcoin/bitcoin/blob/master/src/rpcblockchain.cpp#L448>
(and even then, proxied through a few layers of CCoinView-machinery). In
fact, this line
<https://github.com/bitcoin/bitcoin/blob/master/src/coins.cpp#L48> makes me
suspect that the implementation of GetStats you reference may be dead code.

In any case, you raise a good point: if users of CLevelDBWrapper go
directly for the iterator, they run the risk of dealing with obfuscated
data. This should be remedied somehow.

I'll give it more look this evening.

Thanks again for the find,
James

On Wed, Oct 7, 2015 at 10:25 AM, Daniel Kraft via bitcoin-dev <

Post by Daniel Kraft via bitcoin-dev
Hi!
I hope this is not a stupid question, but I thought I'd ask here first
instead of opening a Github ticket (in case I'm wrong).
With the recently merged "obfuscation" patch, content of the
"chainstate" LevelDB is obfuscated by XOR'ing against a random "key".
This is handled by CLevelDBWrapper's Read/Write methods, which probably
cover most of the usecases.
*However*, shouldn't it also be handled when iterating over the
database? In particular, I would expect that the obfuscation key is
applied before line 119 in txdb.cpp (i. e., while iterating over the
coin database in CCoinsViewDB::GetStats).
Is there a reason why this need not be done there, or is this an actual
oversight?
Yours,
Daniel
--
http://www.domob.eu/
OpenPGP: 1142 850E 6DFF 65BA 63D6 88A8 B249 2AC4 A733 0737
Namecoin: id/domob -> https://nameid.org/?name=domob
--
Done: Arc-Bar-Cav-Hea-Kni-Ran-Rog-Sam-Tou-Val-Wiz
To go: Mon-Pri
_______________________________________________
bitcoin-dev mailing list
https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev

[James O'Beirne via bitcoin-dev]

This has been confirmed as a bug. Thanks again for reporting. I've filed a
fix here (https://github.com/bitcoin/bitcoin/pull/6777), and will be
writing tests to prevent regressions.

Post by James O'Beirne via bitcoin-dev
Hey, Daniel.
Patch author here. Thanks for the diligence; I think this indeed may be an
oversight, though I'm going to need to look into a bit more thoroughly at
home. Curious that it didn't fail any of the automated tests.
Correct me if I'm wrong, but the only actual invocation of that method is
here
<https://github.com/bitcoin/bitcoin/blob/master/src/rpcblockchain.cpp#L448>
(and even then, proxied through a few layers of CCoinView-machinery). In
fact, this line
<https://github.com/bitcoin/bitcoin/blob/master/src/coins.cpp#L48> makes
me suspect that the implementation of GetStats you reference may be dead
code.
In any case, you raise a good point: if users of CLevelDBWrapper go
directly for the iterator, they run the risk of dealing with obfuscated
data. This should be remedied somehow.
I'll give it more look this evening.
Thanks again for the find,
James
On Wed, Oct 7, 2015 at 10:25 AM, Daniel Kraft via bitcoin-dev <

Post by Daniel Kraft via bitcoin-dev
Hi!
I hope this is not a stupid question, but I thought I'd ask here first
instead of opening a Github ticket (in case I'm wrong).
With the recently merged "obfuscation" patch, content of the
"chainstate" LevelDB is obfuscated by XOR'ing against a random "key".
This is handled by CLevelDBWrapper's Read/Write methods, which probably
cover most of the usecases.
*However*, shouldn't it also be handled when iterating over the
database? In particular, I would expect that the obfuscation key is
applied before line 119 in txdb.cpp (i. e., while iterating over the
coin database in CCoinsViewDB::GetStats).
Is there a reason why this need not be done there, or is this an actual
oversight?
Yours,
Daniel
--
http://www.domob.eu/
OpenPGP: 1142 850E 6DFF 65BA 63D6 88A8 B249 2AC4 A733 0737
Namecoin: id/domob -> https://nameid.org/?name=domob
--
Done: Arc-Bar-Cav-Hea-Kni-Ran-Rog-Sam-Tou-Val-Wiz
To go: Mon-Pri
_______________________________________________
bitcoin-dev mailing list
https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev

[Daniel Kraft via bitcoin-dev]

Hi James!

Post by James O'Beirne via bitcoin-dev
This has been confirmed as a bug. Thanks again for reporting. I've filed
a fix here (https://github.com/bitcoin/bitcoin/pull/6777), and will be
writing tests to prevent regressions.

Thanks for the quick fix!

I thought to submit a patch myself today in case the issue is confirmed
as an oversight, but it is very nice to see that this is no longer
necessary at all. :)

Yours,
Daniel