👍 brunoerg approved a pull request: "rpc: Reword SighashFromStr error message"
(https://github.com/bitcoin/bitcoin/pull/29870#pullrequestreview-2015715112)
utACK fa6ab0d020d0b1492203f7eb2ccb8051812de086

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2070869416)
For testing big endian wallets, you can use `db_dump wallet.dat | db_load -c db_lorder=4321 wallet_big_endian.dat` to make big endian databasese using BDB's tools.

However, I also realized that BDB lets us set the endianness to use for a new database, so I've added a `bdb_swap` format to `createfromdump` that will make a BDB database with the other endianness. This will flip it depending on the system that you use, so on little endian, it will make a big endian wallet, and on big endian, it w
...

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1575308063)
> Is this used in this pull request?

No, it's used in the followup.

> I wonder if it could make sense to return the error string to the caller, assuming this is called by RPC eventually?

`Backup` is a part of the `WalletDatabase` parent class, so changing this would also mean changing the `Backup` of existing database classes, and all of their callers. I think that's something that would be worth doing in a followup, but not this PR.

💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2070973893)
cc: @maflcko

💬 ariard commented on pull request "policy: restrict all TRUC (v3) transactions to 25KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2070974638)
> Still, what are TRUCs ? Do you have documentation ?

TRUC = Topologically Restricted Until Confirmation, see https://github.com/bitcoin/bips/pull/1541

💬 achow101 commented on issue "Release schedule for 27.0":
(https://github.com/bitcoin/bitcoin/issues/29028#issuecomment-2071021674)
@levantah Contributions are certainly welcome and desired, you can take a look at any of the open issues, particularly ones tagged as "Good first issue". However, as Bitcoin Core is security software, we only want to link to our own official sources for releases. While we appreciate the gesture you did by hosting a mirror of the release, it is still something we cannot point people to, and for the safety of our users, the comment is removed. While I don't doubt that you are only trying to help,
...

💬 achow101 commented on pull request "Bugfix: bitcoin-cli: Check length of peer.transport_protocol_type":
(https://github.com/bitcoin/bitcoin/pull/29657#issuecomment-2071028198)
ACK c3e632b44153e314ef946f342c68c2758b1cbc4d

🚀 achow101 merged a pull request: "Bugfix: bitcoin-cli: Check length of peer.transport_protocol_type"
(https://github.com/bitcoin/bitcoin/pull/29657)

💬 achow101 commented on pull request "test: Fix intermittent timeout in p2p_tx_download.py":
(https://github.com/bitcoin/bitcoin/pull/29933#issuecomment-2071037489)
ACK fa6c300a9926a1d35fdd0a80f59ea39769bd2596

🚀 achow101 merged a pull request: "test: Fix intermittent timeout in p2p_tx_download.py"
(https://github.com/bitcoin/bitcoin/pull/29933)

💬 achow101 commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1575420781)
In caa5242f84a6e1ef90b8a44c1206c7b5b942a00c "cli: Sanitize ports in rpcconnect and rpcport"

This will fail if you just have a regtest node running independent of the test:

```
2024-04-22T22:30:22.751000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/util.py", line 122, in assert_raises_process_error
fun(*args, **kwds)
File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_fram
...

👍 sdaftuar approved a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2005939457)
Code review ACK (apart from the tests, which I only skimmed). Will test...

💬 sdaftuar commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1568781077)
This is a code style nit, but I think I agree with https://github.com/bitcoin/bitcoin/pull/21062/files#r571975710 that there's not much benefit from using a `std::optional` on `m_replaced_transactions`? It just seems to lead to extra code around understanding when the field is set or not, when I think it would be simpler to say that it's just a list which is either empty or non-empty based on whether a replacement took place.

Anyway I have no objection to the changes in this commit; just wan
...

💬 sdaftuar commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1575419630)
Just an observation: I guess the more common case will likely be multiple children of the same parent transaction (rather than conflicting children). Still, we have no idea which child will have the best chance of successfully bumping it, so this ordering seems as good as any.

💬 sdaftuar commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1575118464)
Let's say we've got a transaction T that is missing inputs. We loop through the missing parents and see that none are in `m_recent_rejects`, so we drop into this block of code.

If there's more than 1 missing parent in `m_recent_rejects_reconsiderable`, is there any benefit to fetching them? It seems like we could tell in advance that validation would fail in that circumstance, because we only try 1P1C packages.

I don't know if it's worth additional complexity to deal with this case, but
...

💬 achow101 commented on pull request "test: remove duplicated ban test":
(https://github.com/bitcoin/bitcoin/pull/29688#issuecomment-2071070836)
It seems like these two tests primarily revolve around the `setban` RPC and do a couple of the same things, so perhaps they can be consolidated into one test?

💬 achow101 commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#issuecomment-2071073647)
ACK 6d91cb781c30966963f28e7577c7aa3829fa9390

🚀 achow101 merged a pull request: "test: refactor: introduce and use `calculate_input_weight` helper"
(https://github.com/bitcoin/bitcoin/pull/29777)

💬 brunoerg commented on pull request "test: remove duplicated ban test":
(https://github.com/bitcoin/bitcoin/pull/29688#issuecomment-2071086936)
> It seems like these two tests primarily revolve around the `setban` RPC and do a couple of the same things, so perhaps they can be consolidated into one test?

See #26863. Closed due to lack of interest.

💬 asctime commented on issue "Crash on fail ~CCheckQueue() destructor?":
(https://github.com/bitcoin/bitcoin/issues/29930#issuecomment-2071114099)
Thanks for your very helpful response maflcko. As you indicate, the code in question was changed anyway, just didn't see the thread. V26.1 is that latest I can compile, haven't updated to c++20. Thanks yes close.