🤔 maflcko requested changes to a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2193273134)
NACK. The last "refactor" push is adding new bugs or behavior changes.

Please keep "refactor" changes to the absolute minimum in real code. (Refactoring tests is fine).

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687617049)
Not sure about changing code that will be obsolete in a few commits. It seems wasteful to waste review on code that will be changed or deleted down the line. It seems better to touch every line of code only once (or the least number of times possible), than to repeatedly refactor it, and then remove the "feature".

Also, this "refactor" (d38bcd39cd98dd45c8b530f415339cda2422ec9f) is introducing new bugs and behavior changes, so it seems like a regression either way.

For example, by removing
...

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687632827)
`-Fragile` feels slightly off to me as well.
`-Unchecked` seems to imply that the function itself may crash.
`-Lenient` feels closer to the truth but sounds safe to use.

I recently came across `MakeRandDeterministicDANGEROUS()`, maybe something like that suffix would work?

💬 maflcko commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2244565876)
> Just to clarify - this option would not be responsible for setting up build targets

I don't care, for me anything works here. Just as a reminder, if the build targets aren't set up at all, someone forgetting to specify `--target fuzz` during the build manually would run into link errors in the other targets (Because there could be conflicts between the fuzz engine's `main`, and the other targets' `main`). (I think those errors are fine, but I wanted to mention it). Also, forgetting `--targe
...

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687643111)
> DANGEROUS

That may also imply the function is crashing? Maybe just `...Deprecated()`?

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687648309)
> Not sure about changing code that will be obsolete in a few commits. It seems wasteful to waste review on code that will be changed or deleted down the line. It seems better to touch every line of code only once (or the least number of times possible), than to repeatedly refactor it, and then remove the "feature".

The function is not being removed as of #30482.

> For example, by removing the `std::fill(m_data.begin(), m_data.end(), 0);`, two `SetHex` calls now may result in a different v
...

💬 maflcko commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2244580379)
> The broadcast pool _only_ contains transactions that Alice has broadcast either from her Core wallet or via the `sendrawtransaction` RPC.

I see. I assumed that Mallory and Alice could have wallet on the same node (multiwallet mode), or that they shared the `sendrawtransaction` RPC on the same node. Not sure if this assumption is accurate, but it seemed plausible that some nodes expose their `sendrawtransaction` to third parties.

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687659582)
I see, the code may be fine then, but I still find it odd to change a line of code repeatedly over time when it can be changed once.

> The function is not being removed as of #30482.

Correct, but parsing less (or more) digits that the data structure can hold does not make sense, so seems best to remove before refactoring this "feature". If there is a use-case, it would be good to mention it, so that it is clear that the refactor is the last time this function needs to be touched.

💬 maflcko commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#issuecomment-2244600650)
> (could you please add me as co-author, `l0rinc <pap.lorinc@gmail.com>`?)


Sure, done

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687667202)
It is dangerous because input isn't length-checked, meaning extra hex chars may be ignored, and an accidentally short copy-paste of hex chars will be silently padded with zeroes.

> Maybe just ...Deprecated()?

As in https://github.com/bitcoin/bitcoin/pull/30482?notification_referrer_id=NT_kwDOCkdNarUxMTU3NjYyODE4MzoxNzI0NDUwMzQ#discussion_r1685726873, I wonder what the policy around deprecation is. Are there prior examples of deprecation on a non-RPC level you can point to?

👍 TheCharlatan approved a pull request: "log: Remove NOLINT(bitcoin-unterminated-logprintf)"
(https://github.com/bitcoin/bitcoin/pull/30485#pullrequestreview-2193365252)
ACK fa18fc705084f3620be566d8c6639b29117ccf68

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687679779)
Only `src/util/time.h`, but those are just type-related; And `src/logging.h`, but those are just aliases; So I don't think they apply here.

Happy to rename it to `SetHexDeprecated`, but I don't think it matters much, as the code is deprecated anyway.

💬 fanquake commented on pull request "depends: build expat with CMake":
(https://github.com/bitcoin/bitcoin/pull/29878#issuecomment-2244662144)
> should we mirror this behaviour?

Which behaviour? If this is missing from one build system, that could be a bug that should be reported upstream. Given that these are stubs compiled just so Qt can compile, I don't think it matters too much.

> Should we bump the CMake minimum required version

Sure. Added a patch.

💬 craigraw commented on issue "Feature Request: Broadcast Pool":
(https://github.com/bitcoin/bitcoin/issues/30471#issuecomment-2244671905)
@vaslid This sounds excellent. From your comment it seems like https://github.com/bitcoin/bitcoin/pull/29415 is a great basis to implement this. I will look into that work more closely, as it's very useful in it's own right. Currently, if Tor is configured, Sparrow broadcasts over Tor via a random external service such as mempool.space for greater privacy. As I understand it, this would no longer be necessary with `-privatebroadcast=1`.

> It is a separate task, which I am planning after https
...

👍 maflcko approved a pull request: "test, assumeutxo: Remove resolved todo comments and add new test"
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2193413701)
ACK d63ef738001fb69ce04134cc8645dcd1e1cbccd1

left a nit (possibly a new test idea, but this can be done later).

💬 maflcko commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1687708137)
nit: I think that test in https://github.com/bitcoin/bitcoin/pull/30267 only checks for a known-invalid header, not for a valid header of a block that later "turns out to be invalid". Also, a test could be added where a block later turns out to have less work, but this is discussed in https://github.com/bitcoin/bitcoin/issues/30288

🤔 glozow reviewed a pull request: "fuzz: Speed up PickValue in txorphan"
(https://github.com/bitcoin/bitcoin/pull/30474#pullrequestreview-2193432107)
ACK fa33a63bd9458f3487a0592983c1363cd30a3c74, thanks for taking the suggestion

💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1687725095)
Thanks for the feedback. I added the `fastprune` option to my local copy and I'm working to understand how pruning operates.

When I tried loading the wallet during background synchronization ([here](https://github.com/bitcoin/bitcoin/pull/30455/files#diff-f763a7cf53e7a5183f66f2634baef84245919265a09d82b03dc435adea8b8faeR149)), I noticed that there wasn't a pruneheight set yet. So, I did a manual prune using `n2.pruneblockchain(START_HEIGHT)`. After pruning, I saw a pruneheight of 300, and the
...

💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1687725271)
Thanks. Will fix it in my next push

💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1687727279)
Thanks. I will address this in my next push along with the other feedback from fjahr