💬 TheCharlatan commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#issuecomment-1669986445)
Re-ACK faaba770e11352ddf6414b9855f4baa46a967580

💬 MarcoFalke commented on pull request "refactor: Enforce C-str fmt strings in WalletLogPrintf()":
(https://github.com/bitcoin/bitcoin/pull/28237#issuecomment-1669987229)
> Am I missing some obvious reason though, other than "might as well"?

Edited OP to list both reasons.

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1287425096)
This is wrong, too. `ScriptPubKeyMan` has a log statement, too.

My recommendation would be to just remove this line completely, unless there is a reason to have it. The other lines should be exact enough to match everything that is needed, without under- or over-matching.

💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1287432683)
Big fan of this rewrite, makes it pretty extensible without too much code duplication. Nice!

Building on your new approach, the non-default parameters could potentially be handled with pointers, to avoid unnecessary copy operations. For example, I think these 2 lines: https://github.com/bitcoin/bitcoin/blob/b565485c24c0feacae559a7f6f7b83d7516ca58d/src/wallet/rpc/spend.cpp#L274-L275

can be rewritten as:
```cpp
if (auto comment{self.Arg<const std::string*>(2)}; comment && !(comment->
...

💬 Subhra264 commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1287435261)
According to the BIP,
> Inputs not specifying a lock time field can take both types of lock times, as can those that specify both.

so if I am not missing anything here and both the `time_lock` and `height_lock` of `psbtin` are `std::nullopt`, shouldn't it be accepted irrespective of `input` lock times?

👍 stickies-v approved a pull request: "rpc: Add Arg() default helper"
(https://github.com/bitcoin/bitcoin/pull/28230#pullrequestreview-1567815521)
ACK fa5468dcf0109d11ddaeb4b3591e6e04b4ea6125, really like this!

💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1287446839)
nit: using `.at(i)` here and `[i]` just a few lines up, when I think they both have the same preconditions, so staying consistent is probably better

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1287458787)
`std::vector<PackageEntry&>` isn't possible, but will do reference wrappers

🤔 instagibbs reviewed a pull request: "validate package transactions with their in-package ancestor sets"
(https://github.com/bitcoin/bitcoin/pull/26711#pullrequestreview-1543557301)
dropping the comments since I haven't seen any approach pushback yet, and I don't want to lose these comments accidentally

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272336717)
very glad to see the special casing of this to MISSING_INPUTS gone

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272306357)
yay! much easier to think about now

needs a reasonable comment to distinguish from TX_MEMPOOL_POLICY

my suggestion was adding something like "but result may differ when in different package"

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1272371805)
We'll need some miniminer Experts(TM) to review this, but I'm just going to assume for now that this will be used as a topological "tie breaker" for linearization that is currently in the branch to this point

so if there's a bug in it, it's no worse than topo-sort, and can be improved later if dependencies need to be cut for whatever reason

💬 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
...