💬 rkrux commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#discussion_r1596591694)
Nit: This portion is copy-paste of the above block, can be extracted in a function `compareMempoolsEntry(entry0, entry1)`

💬 Umiiii commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#discussion_r1596601706)
[See this](https://github.com/bitcoin/bitcoin/pull/21800)

💬 Umiiii commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#discussion_r1596605908)
`mempool1` only stores string(txid), while `getmempoolentry` contains more information that we need to check, so no, it is not redundant. Also see: https://developer.bitcoin.org/reference/rpc/getmempoolentry.html

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596607978)
Same as above, imo definitely want to log txid if we have it.

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596610435)
Yes

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596607638)
Yes, I would definitely say to log both txid and wtxid when we have it. A lot of things use txid only, e.g. if you want to trace a tx through logs or look it up in mempool. Would be a pain to not have txid imo.

💬 Umiiii commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#discussion_r1596612470)
That's on purpose, just to align with the existing coding style within this file (and other testing code). Those unit tests don't actually introduce new functions while comparing two entries (and the entries' variables).

🤔 ismaelsadeeq reviewed a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2049842223)
Code Review ACK 6a39183b53a41bb282ebf8373d0e11691d97d365

💬 AngusP commented on pull request "test: Refactor fee/feerate calculations in feature_fee_estimation.py to use Decimal instead of float":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1596597207)
Nit:
```suggestion
from decimal import Decimal, ROUND_DOWN
```

💬 AngusP commented on pull request "test: Refactor fee/feerate calculations in feature_fee_estimation.py to use Decimal instead of float":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1596617286)
nit:

```suggestion
SATS_PRECISION = Decimal('0.00000001')
...

def satoshi_round(amount: Union[int, float, str], *, rounding: str) -> Decimal:
"""Rounds a Decimal amount to the nearest satoshi using the specified rounding mode."""
return Decimal(amount).quantize(SATS_PRECISION, rounding=rounding)
```

The `Decimal('0.00000001')` appears in a few places, would be good to extract it as a constant `SATS_PRECISION` (or whatever name you prefer) for readability and to make it less
...

💬 AngusP commented on pull request "test: Refactor fee/feerate calculations in feature_fee_estimation.py to use Decimal instead of float":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1596607243)
Default `Decimal` rounding is [`ROUND_HALF_EVEN`](https://docs.python.org/3/library/decimal.html#decimal.ROUND_HALF_EVEN) -- "Round to nearest with ties going to nearest even integer.":

```python
>>> from decimal import getcontext
>>> getcontext().rounding
ROUND_HALF_EVEN
```

I wasn't super sure what you are trying to do here, but this example helps:

```python
>>> Decimal(0.004).quantize(Decimal('0.01'))
Decimal('0.00')

>>> Decimal(0.008).quantize(Decimal('0.01'))
Decimal('0.0
...

💬 rkrux commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#discussion_r1596629322)
You mean in [mempool_package_limits](https://github.com/bitcoin/bitcoin/pull/21800/files#diff-153b2fda84a768c77135f65d26f04fa86262a89f4009f05690aeb47269a418e7)?

💬 rkrux commented on pull request "test: add missing comparison of node1's mempool in MempoolPackagesTest":
(https://github.com/bitcoin/bitcoin/pull/29948#discussion_r1596631691)
Hmm so not redundant keeping in mind that testing `getrawmempool` is also needed, got it.

👍 rkrux approved a pull request: "test: add missing comparison of node1's mempool in MempoolPackagesTest"
(https://github.com/bitcoin/bitcoin/pull/29948#pullrequestreview-2049875497)
Thanks for quick responses.

💬 kristapsk commented on pull request "test: Refactor fee/feerate calculations in feature_fee_estimation.py to use Decimal instead of float":
(https://github.com/bitcoin/bitcoin/pull/29566#issuecomment-2104444054)
Concept ACK

🤔 glozow reviewed a pull request: "p2p: index TxOrphanage by wtxid, allow entries with same txid"
(https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2049866191)
Ok I have split the first commit into:
- net processing change to stop querying by txid
- 2 refactors changing the TxOrphanage methods to be by Wtxid only
- TxOrphanage change to index by wtxid and thus allow entries with the same txid

Hopefully in review, we can convince ourselves that the first commit is the right thing to do, separately from the other changes. The tests may help with that - also added one for invs that helps illustrate what the cases are, and added unit tests.

I thou
...

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596628218)
fixed thanks

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596626751)
done

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596626910)
done

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596628093)
inlined. didn't do the rename as I don't think it's incorrect and want to minimize diff