💬 S3RK commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#discussion_r1250387177)
Isn't the fee for all outputs accounted for in `not_input_fees`? It seems to me this will always overpay and then will be corrected when we detect surplus fee.

Maybe a clearer way to handle this would be to pass `chnage_fee=0` to `GetChange` if `existent_change_out_index=true`. I'd prefer to minimise number of cases when we need to correct fees at the later stage of transaction building.

💬 S3RK commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#issuecomment-1617526433)
How would it work if coinselection produced result without change output?

💬 MarcoFalke commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1617532581)
Yeah, should be caused by an upstream CI change. I guess there should be no risk in changing 120s to 300s, even potentially fixing the issue you see.

🤔 MarcoFalke reviewed a pull request: "test: move remaining rand code from util/setup_common to util/random"
(https://github.com/bitcoin/bitcoin/pull/27425#pullrequestreview-1510582779)
lgtm ACK 5b3f6a49e6bda9f5a0ec261f3f8900e4978e3a1a 🐤

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 5b3f6a49e6bda9f5
...

💬 MarcoFalke commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1250420188)
in 5b3f6a49e6bda9f5a0ec261f3f8900e4978e3a1a:

Some notes:

* I wonder if this should still be called "insecure", as the underlying randomness is no longer insecure, just deterministic, which is obvious in a test env. Maybe call it MockedRandBytes?
* `static` isn't needed?
* I wonder if it makes sense to wrap all member functions to global functions. Why not just call the member function of the global? (Before maybe renaming it to `g_mocked_random` or `g_test_random` or so? This would allow
...

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1246207242)
in "Bump unconfirmed parent txs to target feerate" 8e3d924f02ee28043e8844a301389915823e5893

I think all the tests would be easier to understand if we start with an empty testing wallet and send unconfirmed txs to it from `def_wallet`.

Now we start with the testing wallet with already one confirmed input, but why if we test spending mostly unconfirmed txs? Current setup requires reader to do more coin tracking in mind.

🤔 S3RK reviewed a pull request: "Bump unconfirmed ancestor transactions to target feerate"
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1504669255)
Started reviewing tests, but need more time.

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1246215741)
do we want to check naïve ancestry fee rate or the fee_rate at which it will be accepted at a block (i.e. ancestry score)?

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1246199879)
in "Bump unconfirmed parent txs to target feerate" 8e3d924f02ee28043e8844a301389915823e5893

nit: better use a multiple of `target_fee_rate`

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1250425745)
that seems like a duplicate check of `assert_undershoots_target`

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1250394244)
Oops, sorry about misleading you 🙄

💬 S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1250419037)
My idea was to add `bump_fee_group_discount` within `GetTotalBumpFee()` function. Probably I'm missing something but I don't know what. Why wouldn't it just work?

📝 fanquake opened a pull request: "docs: fixup honggfuzz fuzz patch"
(https://github.com/bitcoin/bitcoin/pull/28021)
Closes #28019.

💬 MarcoFalke commented on pull request "docs: fixup honggfuzz fuzz patch":
(https://github.com/bitcoin/bitcoin/pull/28021#discussion_r1250601396)
Any reason to make the comment wrong?

💬 fanquake commented on pull request "docs: fixup honggfuzz fuzz patch":
(https://github.com/bitcoin/bitcoin/pull/28021#discussion_r1250606783)
Just a miss-copy and paste

👍 MarcoFalke approved a pull request: "docs: fixup honggfuzz fuzz patch"
(https://github.com/bitcoin/bitcoin/pull/28021#pullrequestreview-1510847799)
lgtm

💬 stickies-v commented on issue "rpc: doc: RPCHelpMan missing request parameters for getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/27998#issuecomment-1617805106)
Absolutely, just open the PR whenever you feel ready, or ask away here if you have any problems you can't figure out.

👍 brunoerg approved a pull request: "docs: fixup honggfuzz fuzz patch"
(https://github.com/bitcoin/bitcoin/pull/28021#pullrequestreview-1510913912)
ACK c1247c3746d4b9ea88a0f9cfb7e71904267b3cd3

✅ fanquake closed an issue: "docs: "Fuzzing the Bitcoin Core P2P layer using Honggfuzz NetDriver" is outdated"
(https://github.com/bitcoin/bitcoin/issues/28019)

🚀 fanquake merged a pull request: "docs: fixup honggfuzz fuzz patch"
(https://github.com/bitcoin/bitcoin/pull/28021)