💬 achow101 commented on pull request "sign: don't assume we are parsing a sane TapMiniscript":
(https://github.com/bitcoin/bitcoin/pull/29853#discussion_r1566217127)
I think it would be good to check that `SignSignature` fails here:

```suggestion
BOOST_CHECK(!SignSignature(keystore, CTransaction(prev), curr, 0, SIGHASH_ALL, sig_data));
```

💬 laanwj commented on issue "Intermittent issue in feature_proxy.py AssertionError: not(bytearray(b'node.noumenon') == b'fc00:1:2:3:4:5:6:7')":
(https://github.com/bitcoin/bitcoin/issues/29871#issuecomment-2057480445)
Looks like the two successive testcases in `node_test` are out of step? `[fc00:1:2:3:4:5:6:7]:8888` is from the CJDNS test, `node.noumenon:8333` is the name-based test after it, interfering with each other.

🤔 achow101 reviewed a pull request: "rpc: parse legacy pubkeys consistently with specific error messages"
(https://github.com/bitcoin/bitcoin/pull/28336#pullrequestreview-2001820644)
ACK 98570fe29bb08d7edc48011aa6b9731c6ab4ed2e

💬 achow101 commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1566227161)
In 100e8a75bf5d8196c005331bd8f2ed42ada6d8d0 "rpc: check and throw specific pubkey parsing errors in `HexToPubKey`"

nit: The error message reads a little bit weirdly to me. I think it would make more sense to use "is not" rather than "must be", e.g. "Pubkey abcd is not a hex string".

💬 achow101 commented on pull request "rpc: validate fee estimation mode case insensitive":
(https://github.com/bitcoin/bitcoin/pull/29175#issuecomment-2057510602)
> This is probably more than you wanted to do, but I'd say it would be better to use `FeeModeFromString` to re-use the existing (case-insensitive) parsing code, than to re-implement the parsing and use hard-coded "unset" strings in all call places.

I agree. I think it would make more sense for `FeeModeFromString` to handle "unset" and to check its return value rather than having these repeated string comparisons. It's a bit odd that the estimate mode is being validated in 3 places - `Interpre
...

📝 pinheadmz opened a pull request: "netbase: clean up Proxy logging"
(https://github.com/bitcoin/bitcoin/pull/29882)
Follow up to #27375 and see https://github.com/bitcoin/bitcoin/pull/29649#issuecomment-2057456834

This removes an extra log message when we can't connect to our own proxy. It also removes a log message if the Proxy is invalid, which is unreachable code because we check that during init by calling `SetProxy()`:

https://github.com/bitcoin/bitcoin/blob/d1af4422d13ba84253a1555c2fe5a42170fa8b35/src/netbase.cpp#L663-L666

## Before #27375 if proxy is unreachable

```
2024-04-15T17:54:51Z co
...

💬 alfonsoromanz commented on pull request "Bugfix: bitcoin-cli: Check length of peer.transport_protocol_type":
(https://github.com/bitcoin/bitcoin/pull/29657#issuecomment-2057612784)
ACK c3e632b44153e314ef946f342c68c2758b1cbc4d

⚠️ laanwj opened an issue: "utils: wallet_dump can create a `database` directory, cross-pollinating records"
(https://github.com/bitcoin/bitcoin/issues/29883)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

I created a large directory with wallet dat files for testing #26606. Ran into a weird issue where the output mismatches, but after investigating deeper, this was not due to a bug in that PR.

So I created a script that iterates over all wallets, dumps them with the internal bdb and external bdb, diffs the textual output, stopping on the first wallet which mismatches. Mind that the walle
...

💬 sdaftuar commented on pull request "policy: restrict all TRUC (v3) transactions to 25KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2057668820)
> Would this make things {broken, inconvenient} for potential users of TRUCs, e.g. by requiring swapping between v3 and v2?

Just wanted to add -- I think the case I expect more often would be that users would just create multiple transactions if they had a need to construct a transaction greater than 25kvb (similar to what I think everyone must do today to work around the current 100kvb standardness limit).

The additional transaction overhead from constructing four 25kvb transactions inste
...

✅ laanwj closed an issue: "v27.0 Testing"
(https://github.com/bitcoin/bitcoin/issues/29697)

💬 laanwj commented on issue "v27.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/29697#issuecomment-2057674519)
Closing this issue now that `v27.0` final is tagged.

✅ fanquake closed an issue: "27.0 RC Testing Guide Feedback"
(https://github.com/bitcoin/bitcoin/issues/29685)

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1566348693)
Dropped `uint160`, changed to use an array.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1566349302)
Changed to `int64_t` with casting.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1566349587)
Yes, I've added a check for that.

🤔 sr-gi reviewed a pull request: "net_processing: make any misbehavior trigger immediate discouragement"
(https://github.com/bitcoin/bitcoin/pull/29575#pullrequestreview-2001909800)
A couple of doc nits and a question below

💬 sr-gi commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1566280997)
in: 78515e185755f5eba951917b8e2314ec69446d7d

nit: isn't this 99? (`for i in range(1, NUM_HEADERS)`)

💬 sr-gi commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1566343004)
in: ce238dd79bd3a1190a2229fac4502fa5742fc6e3

I think the comment 6 lines below is wrong now:

```
// It should be impossible for the getheaders request to fail,
// because we should have cleared the last getheaders timestamp
// when processing the headers that triggered this call.
[...]
```

IIUC, `result.request_more` cannot be true if `result.success` is false (based on my understanding of [HeadersSyncState::ProcessNextHeaders](https://github.com/bitcoin/bitcoin/blob/07720b1cdd773
...

💬 sr-gi commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1566293700)
in: ce238dd79bd3a1190a2229fac4502fa5742fc6e3

nit: I found this wording a bit hard to follow, especially the "to make marked responded to" bit.

IIUC, this is sending an empty block as a response to the outstanding getheaders request from the previous loop interaction, otherwise sending `blocks[i]` would be treated as the reply instead. Is that it? If so, I'd suggest something along the lines of:

```suggestion
# Send an empty header as a failed response to the received gethea
...

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-2057691808)
> parsing the headers of overflow pages (which have btree level 0)

Interesting, did not know that they would use an invalid btree level. Fixed as suggested.

***

I've also commented out the enums that we do not use. I decided to leave them there for completeness, but they won't be used.

***

For help with reviewing, I suggest looking at the BDB source code. I've uploaded a copy of the version that we use at https://github.com/achow101/bdb-sources/tree/main/4.8.30.NC. The files that
...