race condition with blockchain.New() writing to wire package variables

[emergent-reasons]

I hesitate to file this. Maybe it is good to be searchable and it also might show a class of race conditions involving package level variables.

The race detector (go test -race ./...) found a race condition:

• blockchain.New() sets some variables in the wire package.
• The detector specifically caught wire.MaxMessagePayload being read and written by two different tests.

That wouldn't matter if, for example, blockchain.New() is only called once, but I found it it used in:

• cmd/addblock/import.go
• cmd/findcheckoint/findcheckpoint.go
• and of course server.go

So there is a remote chance of mangled data using those commands. There also might be a class of problems with package variables.

[cpacia]

This was a change I made as part of Bitcoin Cash consensus rules. The MaxMessagePayload is actually dependent on the ExcessiveBlockSize config option. Given the layout of the codebase I'm not sure how best make it configurable if not a global variable.

Maybe use a sync.Once setter and make it an unexported var that other packages can get by an exported getter.

[zquestz]

Oops sorry about that. I can take a look at a better way to do this tonight.

[zquestz]

@emergent-reasons want to try and fix this one?

[emergent-reasons]

I will take a look.

[emergent-reasons]

Here is what I found:

As Chris said, wire.MaxMessagePayload and other dependent values in wire now depend on a config value, ExcessiveBlockSize

There is an additional bug where wire sets some values based on the wire.MaxMessagePayload default instead of the "final" value set later by wire users.

I came up with a few solutions. If you have a preference or a better solution, please let me know. For now, I will try out 3 to help me learn how much coupling there is.

1. Global ExcessiveBlockSize with an Env variable
   + simple getter with a memo can replace all usages of wire.MaxMessagePayload
   - not sure if it is really a global thing
   - more complicated installation
2. Decouple / redraw data boundary between ExcessiveBlockSize and wire.MaxMessagePayload
   ? I am not familiar enough with the architecture to do this
3. wire.MaxMessagePayload() function that requires the caller to thread in ExcessiveBlockSize
   + perfectly DRY
   + accurate even if ExcessiveBlockSize changes at run time
   - noisy
   - overkill if values will not change at runtime
4. Let wire set wire.MaxMessagePayload by reading ExcessiveBlockSize from config
   + simple in code
   - moving config into its own package leaks a lot of information into all other packages
   - hides instead of removes coupling between main/blockchain/wire
5. Everybody sets wire.MaxMessagePayload (protected by a SetOnce)
   - other packages need to be aware that wire requires initialization (I found 91 files importing it)

[zquestz]

I would investigate 3 and 4. Those seem like most logical options.

[emergent-reasons]

Intermediate results of threading ExcessiveBlockSize into the wire package:

63 files changed, 490 insertions(+), 454 deletions(-)

Har har. Abandon ship. It turns out that the excessiveBlockSize value reaches into just about every function of the Message interface and various private functions. I will look at the config way of doing it.

[cpacia]

@emergent-reasons fwiw I don't think we ever expect to change the excessiveblocksize while running

[emergent-reasons]

Thank you Chris. I didn't think so either. The only gotcha I thought of is that if somebody starts the node with a command line arg for EBS, then the CLI would need to query the server to be sure to have the right behavior. Not sure how big a deal that is.

[emergent-reasons]

Before trying 4. (moving config into a package), I wanted to see what would happen with the easy way 5. (initializing the variable and wrapping with a getter).

It revealed that a lot of the wire limits were static and incorrect. Rather, they were correct but only for EBS of 32,000,000. So there were some failures and worse, some incorrect passes when EBS is changed to another value.

This minimal implementation of 5. (initializing + getter) seems to do the job but it touches a lot of code and as predicted it's a mess due to the requirement of initializing wire for any build that touches it (e.g. tests, commands, main, ...)

If anyone has a minute for critique it would be nice, but I am not planning to PR this. I will try the solution of moving config into it's own package next.