As per title. Refactor for using cdd in tx acceptance later.
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- refactor-priority
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 1623 Build 1623: arc lint + arc unit
Event Timeline
| src/coins.cpp | ||
|---|---|---|
| 311 ↗ | (On Diff #2506) | The IsSpent() seems unnecessary. The inputs have to be validated before this point. The height requirement is also odd as it's always called with the chainTip and coins shouldn't be in the cache unless they've been confirmed. This function should be able to just be gotten rid of in favor of GetValueIn.? |
Comment Actions
Need to dig into out chained unconfirmed transactions interacts with this given that unconfirmed transactions don't seem to be in the coinsviewcache.
| src/coins.cpp | ||
|---|---|---|
| 306 ↗ | (On Diff #2507) | I don't think that returning the variable betting on the fact it is initialized in a certain way make the code very clear. Just return a constant here. |
| 311 ↗ | (On Diff #2507) | When does this check run ? Not passing it would mean we either are seeing a double spend or a transaction that's confirmed in the future ? |
| 325 ↗ | (On Diff #2507) | dito |
| src/coins.h | ||
| 272 ↗ | (On Diff #2507) | Comment is now wrong. |
| src/primitives/transaction.cpp | ||
| 105 ↗ | (On Diff #2507) | It seems rather bizarre to use this function as a member of transaction when it's unable to source its data properly. Why not pass a CCoinsView as a argument to it and do the computation based on it ? |
| src/coins.cpp | ||
|---|---|---|
| 311 ↗ | (On Diff #2507) | Pretty sure both of these checks are already done when the transaction is validated in validation.cpp. (And height is always the last block): I think this entire function should be replaced with just GetValueIn, which is already called above @ My question is in regards to spends chained, and double spends. |
| src/primitives/transaction.cpp | ||
|---|---|---|
| 105 ↗ | (On Diff #2507) | I agree, I had another branch where I was working on that. |
| src/primitives/transaction.cpp | ||
|---|---|---|
| 105 ↗ | (On Diff #2507) | They probably did it this way because of the include order. Most other things depend on CTxOut, so transaction.h can't include much of anything. |
| src/coins.cpp | ||
|---|---|---|
| 319 | Regarding the previous comment, a refactor to put this on CTransaction, where it makes sense, is pretty large. | |
| src/validation.cpp | ||
|---|---|---|
| 785 | I really don't understand where this is going. This is objectively making the API worse because the caller have to fetch some data and then feed it. The whole logic should be either in the view or in the transaction. The coinage really is not up to the caller to chose, it is a function of the transaction and the coinview. | |
| src/validation.cpp | ||
|---|---|---|
| 785 | I don't agree that it is making the API objectively worse, especially when knowing the coinblocks will be useful in the future. Piping the output of one function into another is a pretty standard practice. These are two different APIs and should be split up. You're correct that these should both live on tx, and ComputePriorty should get the blocks destroyed internally, but the way the code is structured that's not currently possible. | |
| src/validation.cpp | ||
|---|---|---|
| 785 | Composition of function is all well and good when they actually compose. There is no composition going on here. ComputePriority has absolutely zero meaning with values other than the coinblock of this very transaction. There is no composition because there is nothing else that can be meaningfully composed with. | |
| src/validation.cpp | ||
|---|---|---|
| 785 | Yes, however CoinBlocks does have other uses. There's no other place in the code to get the age of a transaction. | |
| src/coins.cpp | ||
|---|---|---|
| 319 | It seems like this function should return uint64_t and the cast-to-double done in ComputePriority instead. Returning double here implies that fractional blocks can be destroyed. | |