[Bitcoin-development] 0.4rc1 known bugs

public inbox for bitcoindev@googlegroups.com
 help / color / mirror / Atom feed

* [Bitcoin-development] 0.4rc1 known bugs
@ 2011-09-04  0:13 Gavin Andresen
  2011-09-04  2:43 ` Matt Corallo
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Gavin Andresen @ 2011-09-04  0:13 UTC (permalink / raw)
  To: Bitcoin Dev

Quick status update on 0.4; I probably won't have time to tackle these
properly before Tuesday:

+ sipa found what looks like a deadlock between the addr-handling and
IRC-join-handling code.
+ UukGoblin reports a deadlock problem on a bitcoind handling getwork requests.

If you want to get more familiar with the bitcoin code and you have a
lot of patience, tracking down deadlocks a great way to do it.

+ ArtForz found a performance bug with transactions that have
thousands of inputs and outputs on the solidcoin test network.
 (not as big an issue for bitcoin due to fees being based on
transaction size, but still worrying)

-- 
--
Gavin Andresen



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Bitcoin-development] 0.4rc1 known bugs
  2011-09-04  0:13 [Bitcoin-development] 0.4rc1 known bugs Gavin Andresen
@ 2011-09-04  2:43 ` Matt Corallo
  2011-09-05  7:25 ` Michael Grønager
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Matt Corallo @ 2011-09-04  2:43 UTC (permalink / raw)
  To: bitcoin-development

On Sat, 2011-09-03 at 20:13 -0400, Gavin Andresen wrote:
> Quick status update on 0.4; I probably won't have time to tackle these
> properly before Tuesday:
> 
> + sipa found what looks like a deadlock between the addr-handling and
> IRC-join-handling code.
> + UukGoblin reports a deadlock problem on a bitcoind handling getwork requests.
> 
> If you want to get more familiar with the bitcoin code and you have a
> lot of patience, tracking down deadlocks a great way to do it.
> 
> + ArtForz found a performance bug with transactions that have
> thousands of inputs and outputs on the solidcoin test network.
>  (not as big an issue for bitcoin due to fees being based on
> transaction size, but still worrying)
> 
+ (my fault) Gitian doesnt build properly.




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Bitcoin-development] 0.4rc1 known bugs
  2011-09-04  0:13 [Bitcoin-development] 0.4rc1 known bugs Gavin Andresen
  2011-09-04  2:43 ` Matt Corallo
@ 2011-09-05  7:25 ` Michael Grønager
  2011-09-05 12:42   ` Luke-Jr
       [not found] ` <20110904115926.GA16476@ulyssis.org>
  2011-09-06 20:55 ` Luke-Jr
  3 siblings, 1 reply; 9+ messages in thread
From: Michael Grønager @ 2011-09-05  7:25 UTC (permalink / raw)
  To: Gavin Andresen; +Cc: Bitcoin Dev

Hi Gavin,

Did a quick compile and run (bitcoind, Ubuntu 10.04.3 LTS)

Findings - compile (I do not use the UPNP feature):
in the makefile.unix I have to change the:
USE_UPNP:=0
to 
USE_UPNP:=
i.e. it is defined if it is "0" ! 

running: no apparent issues (I have never managed to trigger the deadlocks.?.)

Nice job, but a quick cleanup of interfaces and classes (one file pr class, all interfaces defined in headers) would really be nice... Would be happy to do it myself, as it would greatly enhance the flexibility of the code and be a first step towards a more library/interface like split.

Cheers,

Michael


On 04/09/2011, at 02:13, Gavin Andresen wrote:

> Quick status update on 0.4; I probably won't have time to tackle these
> properly before Tuesday:
> 
> + sipa found what looks like a deadlock between the addr-handling and
> IRC-join-handling code.
> + UukGoblin reports a deadlock problem on a bitcoind handling getwork requests.
> 
> If you want to get more familiar with the bitcoin code and you have a
> lot of patience, tracking down deadlocks a great way to do it.
> 
> + ArtForz found a performance bug with transactions that have
> thousands of inputs and outputs on the solidcoin test network.
> (not as big an issue for bitcoin due to fees being based on
> transaction size, but still worrying)
> 
> -- 
> --
> Gavin Andresen
> 
> ------------------------------------------------------------------------------
> Special Offer -- Download ArcSight Logger for FREE!
> Finally, a world-class log management solution at an even better 
> price-free! And you'll get a free "Love Thy Logs" t-shirt when you
> download Logger. Secure your free ArcSight Logger TODAY!
> http://p.sf.net/sfu/arcsisghtdev2dev
> _______________________________________________
> Bitcoin-development mailing list
> Bitcoin-development@lists•sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/bitcoin-development

Michael Gronager, PhD
Owner Ceptacle / NDGF Director, NORDUnet A/S
Jens Juels Gade 33
2100 Copenhagen E
Mobile: +45 31 62 14 01
E-mail: gronager@ceptacle•com





^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Bitcoin-development] 0.4rc1 known bugs
  2011-09-05  7:25 ` Michael Grønager
@ 2011-09-05 12:42   ` Luke-Jr
  2011-09-05 12:47     ` Michael Grønager
  0 siblings, 1 reply; 9+ messages in thread
From: Luke-Jr @ 2011-09-05 12:42 UTC (permalink / raw)
  To: bitcoin-development

On Monday, September 05, 2011 3:25:47 AM Michael Grønager wrote:
> Findings - compile (I do not use the UPNP feature):
> in the makefile.unix I have to change the:
> USE_UPNP:=0
> to
> USE_UPNP:=
> i.e. it is defined if it is "0" !

Yes, the default is "UPnP supported, disabled by default" (USE_UPNP=0), not 
"UPnP not supported" (USE_UPNP=). This is documented in build-unix.txt ...



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Bitcoin-development] 0.4rc1 known bugs
  2011-09-05 12:42   ` Luke-Jr
@ 2011-09-05 12:47     ` Michael Grønager
  0 siblings, 0 replies; 9+ messages in thread
From: Michael Grønager @ 2011-09-05 12:47 UTC (permalink / raw)
  To: Luke-Jr; +Cc: bitcoin-development

Sorry, by bad - first clean checkout for quite a while (must have changed it earlier myself...).

/M

On 05/09/2011, at 14:42, Luke-Jr wrote:

> On Monday, September 05, 2011 3:25:47 AM Michael Grønager wrote:
>> Findings - compile (I do not use the UPNP feature):
>> in the makefile.unix I have to change the:
>> USE_UPNP:=0
>> to
>> USE_UPNP:=
>> i.e. it is defined if it is "0" !
> 
> Yes, the default is "UPnP supported, disabled by default" (USE_UPNP=0), not 
> "UPnP not supported" (USE_UPNP=). This is documented in build-unix.txt ...
> 
> ------------------------------------------------------------------------------
> Special Offer -- Download ArcSight Logger for FREE!
> Finally, a world-class log management solution at an even better 
> price-free! And you'll get a free "Love Thy Logs" t-shirt when you
> download Logger. Secure your free ArcSight Logger TODAY!
> http://p.sf.net/sfu/arcsisghtdev2dev
> _______________________________________________
> Bitcoin-development mailing list
> Bitcoin-development@lists•sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/bitcoin-development

Michael Gronager, PhD
Owner Ceptacle / NDGF Director, NORDUnet A/S
Jens Juels Gade 33
2100 Copenhagen E
Mobile: +45 31 62 14 01
E-mail: gronager@ceptacle•com





^ permalink raw reply	[flat|nested] 9+ messages in thread

[parent not found: <20110904115926.GA16476@ulyssis.org>]

* Re: [Bitcoin-development] 0.4rc1 known bugs
       [not found] ` <20110904115926.GA16476@ulyssis.org>
@ 2011-09-06 11:55   ` Pieter Wuille
  2011-09-06 17:59     ` Gavin Andresen
  0 siblings, 1 reply; 9+ messages in thread
From: Pieter Wuille @ 2011-09-06 11:55 UTC (permalink / raw)
  To: Gavin Andresen; +Cc: Bitcoin Dev

On Sun, Sep 4, 2011 at 13:59, Pieter Wuille
<pieter.wuille@cs•kuleuven.be> wrote:
> I've compiled bitcoind with Gavin's DEBUG_LOCKORDER, and fixed two potential
> reported deadlock issues (see https://github.com/sipa/bitcoin/commits/lockfixes).

My mistake: these are not actual potential deadlocks, as all locking
of cs_vRecv/cs_vSend
happens inside TRY_CRITICAL_SECTION blocks. Gavin, maybe you can add the rule to
your debug code that ignores critical sections which are only locked
through TRY_...?

>> + sipa found what looks like a deadlock between the addr-handling and
>> IRC-join-handling code.

Regarding the actual deadlock between IRC seeding and AddAddress:

Internally, DB also uses pthreads to implement the txn_begin()/commit() scheme,
though I'm not sure with which granularity. These need to be taken into account
when searching for deadlocks, but are obviously not detected by
DEBUG_LOCKORDER.

In particular here, the processing of "addr" created a db transaction for the
entire message, while only locking cs_mapAddresses inside AddAddress. For
IRC seeded addresses however, no db tx was precreated, and AddAddress first
locked cs_mapAddress, and then did the database write (causing a lock) inside.

A solution: in main.cpp, ProcessMessage, case "addr":

          // Store the new addresses
          CAddrDB addrDB;
+         CRITICAL_BLOCK(cs_mapAddresses) {
          addrDB.TxnBegin();
          int64 nNow = GetAdjustedTime();
          int64 nSince = nNow - 10 * 60;

              }
          }
          addrDB.TxnCommit();  // Save addresses (it's ok if this fails)
+         }
          if (vAddr.size() < 1000)

which makes sure that cs_mapAddresses is always entered before starting
a database transaction.

However, there may be similar issues in other place where TxnBegin is called
explicitly. Also, maybe there are other solutions, like changing BDB parameters
that make the db transaction fail instead of block, for example.

-- 
Pieter

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Bitcoin-development] 0.4rc1 known bugs
  2011-09-06 11:55   ` Pieter Wuille
@ 2011-09-06 17:59     ` Gavin Andresen
  0 siblings, 0 replies; 9+ messages in thread
From: Gavin Andresen @ 2011-09-06 17:59 UTC (permalink / raw)
  To: Bitcoin Dev

Nice work, Detective Wuille!

Patch for the deadlock issue:

https://github.com/bitcoin/bitcoin/pull/500

I took a different approach to fix from the one Pieter suggested,
performing the database operation after the cs_mapaddresses deadlock
is released.  Please review to check my logic, it did survive my
start/stop/restart... stress test.

And I did review every place in the code that starts a database
transaction, to look for similar issues, and they are all OK.

RE: improving DEBUG_LOCKORDER:  requires some thought.  Deadlocks are
still possible with TRY_CRITICAL_SECTION, if some codepaths TRY and
some don't.

On Tue, Sep 6, 2011 at 7:55 AM, Pieter Wuille
<pieter.wuille@cs•kuleuven.be> wrote:
> My mistake: these are not actual potential deadlocks, as all locking
> of cs_vRecv/cs_vSend
> happens inside TRY_CRITICAL_SECTION blocks. Gavin, maybe you can add the rule to
> your debug code that ignores critical sections which are only locked
> through TRY_...?
>
>>> + sipa found what looks like a deadlock between the addr-handling and
>>> IRC-join-handling code.
>
> Regarding the actual deadlock between IRC seeding and AddAddress:
>
> Internally, DB also uses pthreads to implement the txn_begin()/commit() scheme,
> though I'm not sure with which granularity. These need to be taken into account
> when searching for deadlocks, but are obviously not detected by
> DEBUG_LOCKORDER.

-- 
--
Gavin Andresen

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Bitcoin-development] 0.4rc1 known bugs
  2011-09-04  0:13 [Bitcoin-development] 0.4rc1 known bugs Gavin Andresen
                   ` (2 preceding siblings ...)
       [not found] ` <20110904115926.GA16476@ulyssis.org>
@ 2011-09-06 20:55 ` Luke-Jr
  2011-09-07 15:07   ` Gavin Andresen
  3 siblings, 1 reply; 9+ messages in thread
From: Luke-Jr @ 2011-09-06 20:55 UTC (permalink / raw)
  To: bitcoin-development

Got a fix for the encrypted-wallet mining issue:
- unique_coinbase

It depends on (and merges) the getwork_dedupe fix already common on pools and 
other miners who pay attention to the latest mining fixes.

To merge:
  git fetch git://gitorious.org/~Luke-Jr/bitcoin/luke-jr-bitcoin.git \
    unique_coinbase && git merge FETCH_HEAD



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Bitcoin-development] 0.4rc1 known bugs
  2011-09-06 20:55 ` Luke-Jr
@ 2011-09-07 15:07   ` Gavin Andresen
  0 siblings, 0 replies; 9+ messages in thread
From: Gavin Andresen @ 2011-09-07 15:07 UTC (permalink / raw)
  To: Luke-Jr; +Cc: bitcoin-development

On Tue, Sep 6, 2011 at 4:55 PM, Luke-Jr <luke@dashjr•org> wrote:
> Got a fix for the encrypted-wallet mining issue:
> - unique_coinbase

Turned this into a pull request:
  https://github.com/bitcoin/bitcoin/pull/505

I reviewed the code but have not tested.

Rough sketch of a test plan:

Run clean testnet-in-a-box bitcoind, with -keypool=1
Encrypt the wallet
Run bitcoind getnewaddress until it tell you keypool is exhausted
Generate a couple of blocks via internal miner -- verify: coinbase
transactions have unique txids even though they pay-to default key
Generate a couple of blocks via getwork RPC call -- verify: coinbase
transactions have unique txids


-- 
--
Gavin Andresen



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2011-09-07 15:07 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-04  0:13 [Bitcoin-development] 0.4rc1 known bugs Gavin Andresen
2011-09-04  2:43 ` Matt Corallo
2011-09-05  7:25 ` Michael Grønager
2011-09-05 12:42   ` Luke-Jr
2011-09-05 12:47     ` Michael Grønager
     [not found] ` <20110904115926.GA16476@ulyssis.org>
2011-09-06 11:55   ` Pieter Wuille
2011-09-06 17:59     ` Gavin Andresen
2011-09-06 20:55 ` Luke-Jr
2011-09-07 15:07   ` Gavin Andresen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox