💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272419757)
> (a.fee * a.vsize > b.fee * a.vsize)

fix comment

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272355103)
"[refactor] fill in results for every tx in AcceptPackage"

It could have just been submitted in the above loop, maybe if this goes away later it won't be worth being more verbose...

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272429657)
would be nice to put "Filtered" in the function names of whatever is doing filtering to reduce mental load

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272491372)
Seems like this will continue evaluating individual txns that have no in-package ancestors(that aren't accepted individually before!). Seems safe, but wasn't immediately obvious to me. It will abort any *package* evaluation later.

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272481752)
if this somehow happens in prod, do we want to keep stumbling along?

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272542996)
I think we decided pre-cluster mempool we can't really nail all these cases due to inherent symmetry.

With this rule we can see cases where the penultimate child is paid for by parents, the penultimate child and final child being immediately trimmed next at below-minfee rates. It's not very pathological since the evicted package is below minfee and if the mempool isn't too large it is worthwhile to mine. I think we agreed these can still exist, but just noting.

💬 theuni commented on pull request "refactor: Enforce C-str fmt strings in WalletLogPrintf()":
(https://github.com/bitcoin/bitcoin/pull/28237#discussion_r1287466821)
Hmm.

I haven't tried this using the c-i scripts, but running clang-tidy manually against current master, this works fine for me as-is:
```bash
$ /opt/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang-tidy-16 --load=/home/cory/dev/bitcoin2/contrib/devtools/bitcoin-tidy/build2/libbitcoin-tidy.so -checks="-*,bitcoin-*" wallet/wallet.cpp -- -DHAVE_BUILD_INFO -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I./leveldb/include -isystem /home/cory/dev/bitcoin2/depends
...

💬 YellowRoseCx commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1670067500)
You'd likely have to go back to 2016 or before , when RBF wasn't added, to find the lists of merchants who accepted 0-conf transactions for smaller IRL transactions. A lot of merchants stopped accepting 0- conf when it was made easier to fraudulently cancel a transaction which has overall likely slowed down IRL merchant adoption and use, if not reversed it.



>Users can't make decision on their own

are you serious? "Bitcoin users are dumb and us devs know what's best for them" is a real
...

💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1287493545)
Ugh, I was inclined to argue with you because I think the tests should be as tight as possible. But realistically, it's more likely for a class to be forgotten in the checks (as has happened here) than a false-positive in some future class. So I begrudgingly agree.

💬 ALIR86 commented on pull request "p2p: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1670094367)
I install in a we3 new wallet ,I have 2 unit 2 BTC in my profile I don't underestand your means that you haven't authorized to merge ,merge in a blockchain else just there unfortunately someone with now of forks of staking withdrawals my coins zBzTC legally me swhouse and get profit of staking of forks it hummm?! Truly interest

💬 ALIR86 commented on pull request "p2p: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1670100363)
Why to offer you why I donot are authorized to merge this pull request tell me with now who use of my forks and staking my profile 2 unit BTC me?@'📈🤔🥵☹️ @

🤔 furszy reviewed a pull request: "fuzz: improve `coinselection`"
(https://github.com/bitcoin/bitcoin/pull/27585#pullrequestreview-1566336739)
Code review ACK 1e0f5f088.

💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1287502527)
what about asserting inputs are actually added and checking the total weight?

💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1286505964)
Could also check the resulting inputs size and the `use_effective_fee` field.

Also, we only use of the `Merge` function in the sources to combine a `MANUAL` selection with a one of the automatic selections. We never combine two srd solutions.

💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1287503891)
what was the reason behind this change?

🤔 pinheadmz reviewed a pull request: "p2p: Allow whitelisting outgoing connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1567795941)
code review ACK at d54e7dad7513ab4c587456b56d3a838daa151e4d

built and passed all tests

However as noted below I think the approach can be simplified in regards to local services.

💬 pinheadmz commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1287433864)
I don't think you need to use `sprintf` here? Might be left over from the `incoming only` thing you just updated?

💬 pinheadmz commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1287479200)
I might be misunderstanding something here, but I think this chunk can be moved to `net_processing.cpp` `InitializeNode()` because:
- the `CNode` will already have its `m_permission_flags` set in `ConnectNode()`
- `m_permission_flags` is all that's needed to set `our_services` inside `InitializeNode()`
- then you won't need to use `std::optional<std::pair<CNode*, ServiceFlags>> ` which is, hey, a real banger of a type!

Otherwise I don't see a reason to compute and then pass around those se
...

💬 hebasto commented on pull request "Release `LockData::dd_mutex` before calling `*_detected` functions":
(https://github.com/bitcoin/bitcoin/pull/26781#issuecomment-1670156374)
cc @ryanofsky

💬 TheCharlatan commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#issuecomment-1670237772)
Concept ACK