💬 paplorinc commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#issuecomment-2271270916)
ACK 59c0ece0a785ce9e22fbfefce9ca228d85e5d43d

💬 brunoerg commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#issuecomment-2271271424)
Concept ACK

💬 willcl-ark commented on pull request "rename TransactionError:ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#issuecomment-2271278295)
* Dropped the first commit based on feedback recieved.
* Dropped the final commit, as following merge of #29855, I don't think that we can actually hit a `PSBTError::MISSING_INPUTS` any more, as we return early. #29855 also modified the only test we had for a `MISSING_INPUTS` error, and I can't trivially see a codepath to trigger it any more. Will address this in a followup.

🤔 marcofleon reviewed a pull request: "fuzz: replace hardcoded numbers for bech32 limits"
(https://github.com/bitcoin/bitcoin/pull/30596#pullrequestreview-2221309312)
ACK 59c0ece0a785ce9e22fbfefce9ca228d85e5d43d. Ran the test a bit to be sure, lgtm.

💬 murchandamus commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2271296632)
Agree with @petertodd’s assessment: I don’t think this should be included in v28.0. It might be nice to issue a deprecation warning in v28 on the startup parameter, though.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1705542508)
Good idea, updated `fuzz/hex.cpp` to cover `result_size` with an `int16_t`. Not sure what the optimal range of values would be here, but I figured not making it too large (`int`) makes sense given that `result_size` is not user input?

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2271300910)
@murchandamus feel free to open the deprecation PR :+1: I'm leaving as draft as noted in OP until post-28

💬 willcl-ark commented on issue "docs: Windows build intructions result in a large binary":
(https://github.com/bitcoin/bitcoin/issues/30593#issuecomment-2271304490)
In a post-cmake world this is the type of thing I would expect developers to add to their own [cmakuserpresets.json](https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html), although I don't see any reason it couldn't be documented too. Compiling without debug symbols is already documented in build-unix.md:

https://github.com/bitcoin/bitcoin/blob/43740f4971f45cd5499470b6a085b3ecd8b96d28/doc/build-unix.md?plain=1#L30

But even building with `CFLAGS` and `CXXFLAGS` leaves some info `
...

💬 ismaelsadeeq commented on pull request "doc, rpc : `#30275` followups":
(https://github.com/bitcoin/bitcoin/pull/30525#discussion_r1705571908)
Thanks I removed the last commit to focus this PR on #30275 follow-up.

🤔 glozow reviewed a pull request: "doc, rpc : `#30275` followups"
(https://github.com/bitcoin/bitcoin/pull/30525#pullrequestreview-2221386015)
ACK fa2f26960ee084971ab09959b213a9b8104482e5

👍 brunoerg approved a pull request: "fuzz: replace hardcoded numbers for bech32 limits"
(https://github.com/bitcoin/bitcoin/pull/30596#pullrequestreview-2221391468)
utACK 59c0ece0a785ce9e22fbfefce9ca228d85e5d43d

🚀 glozow merged a pull request: "fuzz: replace hardcoded numbers for bech32 limits"
(https://github.com/bitcoin/bitcoin/pull/30596)

💬 sipa commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705601751)
@josibake The keyword `static` in C++ has two completely different meanings sadly :angry:

Inside a class, a `static` variable/function makes it a class member rather than an instance member (shared by all instances).

Outside a class, `static` means that a variable/function is only accessible within its compilation unit (whose effect is identical to placing it in an anonymous `namespace`).

💬 paplorinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1705621067)
I managed to make it work, I think the internal IDE setting were overwriting the ones I was setting from the command line.

For the record, with these settings the bench and fuzz projects are enabled (though no IDE assistance - i.e. run from the IDE -, but that's a different issue):
<img src="https://github.com/user-attachments/assets/06b10237-c4b3-4ca3-8547-b3299d214070">

I did open an unrelated suggestion for better error messages: https://github.com/hebasto/bitcoin/issues/306

💬 ryanofsky commented on issue "TransactionError::ALREADY_IN_CHAIN inconsistency ":
(https://github.com/bitcoin/bitcoin/issues/19363#issuecomment-2271462243)
> I think that `TransactionError::ALREADY_IN_CHAIN` should be returned in all cases when the transaction is in the block chain, or it should be dropped from the code at all as inconsistent.

#30212 implements the second option. It gets rid of already in chain error code since that case can't be reliably detected, and returns already in utxo set, or missing inputs instead.

👍 ryanofsky approved a pull request: "rename TransactionError:ALREADY_IN_CHAIN"
(https://github.com/bitcoin/bitcoin/pull/30212#pullrequestreview-2221494879)
Code review ACK e9de0a76b99fd4708295e1178f6c079db92e7f36.

Since last review, change has been simplified, no longer renaming `PSBTError::MISSING_INPUTS` to `PSBTError::INPUTS_INVALID` and no longer renaming `TransactionError::MISSING_INPUTS` to `TransactionError::INPUTS_MISSING_OR_SPENT`

I did think both of those renames were improvements, but it does simplify the PR to drop them. Also it makes it easier to see how this PR fixes #19363.

Also release notes were added

💬 ryanofsky commented on pull request "rename TransactionError:ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1705652658)
In commit "rpc: clarify ALREADY_IN_CHAIN rpc errors" (87b188052528e97729a85d9701864eaff1ea5ec6)

Was already the case before this PR, but this documentation seems misleading because "already in utxo set" error is not the only error that can be triggered if transaction can't be added to the mempool.

As #19363 reports BroadcastTransaction will start returning MISSING_INPUTS instead of ALREADY_IN_UTXO_SET after the outputs are spent. Also it looks like BroadcastTransaction can returns MEMPOOL
...

💬 josibake commented on pull request "fuzz: replace hardcoded numbers for bech32 limits":
(https://github.com/bitcoin/bitcoin/pull/30596#discussion_r1705674254)
@sipa , @paplorinc thanks for the explanation! In this case, since we are in a _named_ namespace, it seems the static keyword would be preferable? Or doesn't matter? Will do some more reading on this cause I'm not sure what the best practice is here.

> Separating constants and definitions into the header, leaving implementations in the cpp, i.e. separation of concerns.

This is another one I'm not sure about: in my mind, `CHARSET` is internal to bech32 hence why it makes sense to keep it in
...

💬 instagibbs commented on pull request "docs: doc update for mempoolfullrbf default + log deprecation":
(https://github.com/bitcoin/bitcoin/pull/30594#discussion_r1705685573)
```Suggestion
1. If `-mempoolfullrbf=0` (the value is 1 by default), the directly conflicting transactions all signal replaceability explicitly. A transaction is
Comment

```

💬 instagibbs commented on pull request "docs: doc update for mempoolfullrbf default + log deprecation":
(https://github.com/bitcoin/bitcoin/pull/30594#discussion_r1705686897)
```Suggestion
- If `-mempoolfullrbf=0` (the value is 1 by default), all direct conflicts must signal replacement.
```