💬 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

👍 vasild approved a pull request: "i2p: fix and improve logs"
(https://github.com/bitcoin/bitcoin/pull/29833#pullrequestreview-2049903727)
ACK 469e4834bad295f6d9fbf8048a8db23aa758bac9

💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596649444)
Split up the commit, hope it helps (https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2049866191)

💬 cbergqvist commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1596649820)
Adding 5 to the timeout fixes it for me. Modifying the line with the mocktime does not (I also tried removing that line which caused the test to fail again, proving its usefulness).
```
with node.assert_debug_log(expected_msgs=[f"Couldn't connect to peers from addrman after {ADD_NEXT_SEEDNODE} seconds. Adding seednode ({seed_node}) to addrfetch"], unexpected_msgs=["Empty addrman, adding seednode"], timeout=ADD_NEXT_SEEDNODE + 5):
self.restart_node(0, extra_args=[f'-seednod
...

🤔 ismaelsadeeq reviewed a pull request: "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`"
(https://github.com/bitcoin/bitcoin/pull/29939#pullrequestreview-2049928550)
Post merge Tested ACK dd8fa861939d5b8bdd844ad7cab015d08533a91a

💬 rkrux commented on pull request "test: fix MiniWallet script-path spend (missing parity bit in leaf version)":
(https://github.com/bitcoin/bitcoin/pull/30076#discussion_r1596687155)
I feel we should tie this `only-path` here to `taproot_construct` call in `address.py`, either by passing it as a parameter or creating it as a constant. Otherwise, it makes you wonder here at this line where did `only-path` come from.